Introduce the `generateMask` option #1990

lpinca · 2021-12-19T19:53:22Z

The generateMask option specifies a function that can be used to
generate custom masking keys.

Refs: #1986
Refs: #1988
Refs: #1989

lpinca · 2021-12-19T20:10:51Z

lib/sender.js


    target[1] |= 0x80;
    target[offset - 4] = mask[0];
    target[offset - 3] = mask[1];
    target[offset - 2] = mask[2];
    target[offset - 1] = mask[3];

+    if ((mask[0] | mask[1] | mask[2] | mask[3]) === 0) {


I think this optimization does not make sense. It is actually counterproductive in the common case. If a random mask is used, then the probability of this event is 1 / 2**32. The same applies for the same optimization on lib/receiver.js.

I don’t think it hurts and allows for hot path optimization for clients that know the hot path “trick”.

I initially called the option fillMaskBuffer, and made it take the mask buffer like crypto.randomFillSync(). Then I changed my mind and went for generateMask. The reason is that it is simpler to use. The user does not have to remember to fill the buffer. They are not forced to do that but we should then document that the last one is used even if the WebSocket instance is different (this might be confusing). With generateMask, not returning a buffer results in a thrown error. Finally, generateMask does not force the user to return a new Buffer instance every time. The user can return the same instance:

const mask = Buffer.alloc(4); const ws = new Websocket(url, { generateMask() { mask.fill(0); return mask; } });

Isn't this sufficient? I don't think ws will mutate the buffer, right?

const mask = Buffer.alloc(4); const ws = new Websocket(url, { generateMask() { return mask; } });

Correct, ws does not mutate it.

@e3dio

I benchmarked this line (mask[0] | mask[1] | mask[2] | mask[3]) === 0 and it came out to less than 1 nanosecond

How can you get that precision? Two sequential process.hrtime.bigint() with nothing in-between take ~0.1 milliseconds / ~100000 nanoseconds on my machine.

Why don't you make an uint32array from the memory and just check if (a[0])

Ok sorry I see what you are saying, pass around Uint32Array's instead of Buffers for the mask:

uint32 = new Uint32Array(1) // new mask default value = 0 { generateMask: uint32 => require('crypto').randomFillSync(uint32) } { generateMask: uint32 => uint32[0] = Math.random()*2**32 } { generateMask: uint32 => uint32[0]++ } const skipMasking = uint32[0] === 0;

They are easier to work with than Buffer because the mask is 32 bit unsigned int. Maybe only reason to use Buffer is more people are familiar with Buffer? You could do either one, Uint32Array makes more sense, you just have to know how it works. Buffer is easier to say than Uint32Array. Ideally Uint32Array but Buffer is ok. Uint32Array has a performance advantage setting 1 value vs 4

Maybe only reason to use Buffer is more people are familiar with Buffer?

Yes, and because bufferUtil.mask() was written to take a buffer at the time https://github.com/websockets/bufferutil/blob/cd48704aa8e87c19d7497812d466fd6fb6a28c09/src/bufferutil.cc#L89-L90, when typed arrays were not supported, and that did not change over time. We could make it take a Uint32Array but I do not think it is worth the effort.

e3dio · 2021-12-19T21:54:30Z

lib/sender.js

+    let mask = _mask;
+
+    if (options.generateMask) {
+      mask = options.generateMask();


This generates a new Buffer object on every send, instead it should modify existing buffer

Not necessarily, see #1990 (comment).

It makes the config setting awkward to require maintaining the buffer outside the setting, it would be more convenient and succinct to pass the buffer to the function:

new WebSocket(url, { generateMask: buf => require('crypto').randomFillSync(buf) } )

it would be more convenient and succinct to pass the buffer to the function.

The only reason why I did not do that is that the buffer is shared between WebSocket instances and this might generate confusion even if it is documented because people do not read.

Does it need to be shared?

It requires more changes because I don't want to add a mask buffer also for server clients and currently the Sender is server/client agnostic.

There is also the static Sender.frame() method. It gets kinda messy.

You only need to create an extra Buffer if the Sender has the config option generateMask set, otherwise you use the shared buffer

See 8ab0db4

Looks like exactly what is needed for that, this would be a great feature nice work

ronag · 2021-12-20T08:18:01Z

LGTM! Thanks!

ronag

I think allocating target is not necessary on the === 0 fast path?

The `generateMask` option specifies a function that can be used to generate custom masking keys. Refs: #1986 Refs: #1988 Refs: #1989

lpinca · 2021-12-20T12:06:49Z

I think allocating target is not necessary on the === 0 fast path?

It is, but target length should be offset. This is not the case if merge is true. It must be fixed. merge should be false if the masking key is zero.

e3dio · 2021-12-20T12:13:40Z

I had to come back and fix that on my 1st PR 9a22689

added PR suggestion: https://github.com/websockets/ws/pull/1991/files

lpinca · 2021-12-20T12:17:07Z

Will fix it later, when I get a chance.

lpinca · 2021-12-20T20:05:50Z

Thank you for the reviews.

lpinca force-pushed the add/generate-mask-option branch from c15291a to 95f269d Compare December 19, 2021 19:55

lpinca changed the title ~~Introduce the generateMask option~~ Introduce the generateMask option Dec 19, 2021

lpinca mentioned this pull request Dec 19, 2021

Add ability to specify the masking key #1989

Closed

lpinca commented Dec 19, 2021

View reviewed changes

ronag approved these changes Dec 19, 2021

View reviewed changes

e3dio reviewed Dec 19, 2021

View reviewed changes

lpinca force-pushed the add/generate-mask-option branch 2 times, most recently from 6564661 to 18bef28 Compare December 20, 2021 10:43

ronag approved these changes Dec 20, 2021

View reviewed changes

lpinca force-pushed the add/generate-mask-option branch from 18bef28 to 8fb2fc1 Compare December 20, 2021 10:53

ronag reviewed Dec 20, 2021

View reviewed changes

[feature] Introduce the generateMask option

17e2554

The `generateMask` option specifies a function that can be used to generate custom masking keys. Refs: #1986 Refs: #1988 Refs: #1989

lpinca force-pushed the add/generate-mask-option branch from 8fb2fc1 to 4da9b64 Compare December 20, 2021 11:43

lpinca force-pushed the add/generate-mask-option branch from 4da9b64 to d0e5b34 Compare December 20, 2021 17:49

[perf] Skip masking and unmasking if the masking key is zero

Verified

This commit was signed with the committer’s verified signature.

lukekarrys Luke Karrys

SSH Key Fingerprint: tdR2129cIUhOKMlhHfQ2KdWrwNRNDT0jNNCQR+Mbb3g
Verified
Learn about vigilant mode

6d83e71

lpinca force-pushed the add/generate-mask-option branch from d0e5b34 to 6d83e71 Compare December 20, 2021 18:15

lpinca merged commit 35d45c2 into master Dec 20, 2021

lpinca deleted the add/generate-mask-option branch December 20, 2021 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce the `generateMask` option #1990

Introduce the `generateMask` option #1990

lpinca commented Dec 19, 2021

lpinca Dec 19, 2021

ronag Dec 19, 2021 •

edited

Loading

lpinca Dec 20, 2021 •

edited

Loading

ronag Dec 20, 2021

lpinca Dec 20, 2021 •

edited

Loading

lpinca Dec 20, 2021 •

edited

Loading

ghost Dec 20, 2021

e3dio Dec 21, 2021 •

edited

Loading

lpinca Dec 21, 2021 •

edited

Loading

e3dio Dec 19, 2021

lpinca Dec 20, 2021 •

edited

Loading

e3dio Dec 20, 2021 •

edited

Loading

lpinca Dec 20, 2021 •

edited

Loading

ronag Dec 20, 2021

lpinca Dec 20, 2021

lpinca Dec 20, 2021

e3dio Dec 20, 2021

lpinca Dec 20, 2021

e3dio Dec 20, 2021

ronag commented Dec 20, 2021

ronag left a comment

lpinca commented Dec 20, 2021 •

edited

Loading

e3dio commented Dec 20, 2021 •

edited

Loading

lpinca commented Dec 20, 2021

lpinca commented Dec 20, 2021

Introduce the generateMask option #1990

Introduce the generateMask option #1990

Conversation

lpinca commented Dec 19, 2021

Choose a reason for hiding this comment

ronag Dec 19, 2021 • edited Loading

Choose a reason for hiding this comment

lpinca Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

lpinca Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e3dio Dec 21, 2021 • edited Loading

Choose a reason for hiding this comment

lpinca Dec 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

e3dio Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

lpinca Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronag commented Dec 20, 2021

ronag left a comment

Choose a reason for hiding this comment

lpinca commented Dec 20, 2021 • edited Loading

e3dio commented Dec 20, 2021 • edited Loading

lpinca commented Dec 20, 2021

lpinca commented Dec 20, 2021

Introduce the `generateMask` option #1990

Introduce the `generateMask` option #1990

ronag Dec 19, 2021 •

edited

Loading

lpinca Dec 20, 2021 •

edited

Loading

lpinca Dec 20, 2021 •

edited

Loading

lpinca Dec 20, 2021 •

edited

Loading

e3dio Dec 21, 2021 •

edited

Loading

lpinca Dec 21, 2021 •

edited

Loading

lpinca Dec 20, 2021 •

edited

Loading

e3dio Dec 20, 2021 •

edited

Loading

lpinca Dec 20, 2021 •

edited

Loading

lpinca commented Dec 20, 2021 •

edited

Loading

e3dio commented Dec 20, 2021 •

edited

Loading