Breaking changes #1904

lpinca · 2021-06-18T19:45:12Z

I'm opening this PR to get a feedback on some breaking changes that I have been delaying for a while and planning to release in a new major version soon.

lpinca · 2021-07-02T16:36:01Z

I'm not sure if ab7b6d4 makes sense. It is good for performance but adapting existing code to this change might require a little effort. Users have call buffer.toString() themselves:

ws.on('message', function (data, isBinary) {
  const message = isBinary ? data : data.toString();
  // Continue as before.
});

ws.on('close', function (code, data) {
  const reason = data.toString();
  // continue as before.
});

If anyone is watching this, please share your opinions.

jimmywarting · 2021-07-05T01:33:36Z

I'm not sure if ab7b6d4 makes sense. It is good for performance but adapting existing code to this change might require a little effort. Users have call buffer.toString() themselves

I say: emit a ArrayBuffer (or Blob) & follow the spec instead do not emit Buffer or strings, do what is best for cross platform coding, ppl should build there application so they work in Deno or browser the same way

if you are planing on making a breaking change anyway, what do you feel about switching to ESM? node "^12.20.0 || ^14.13.0"

While we are at it, go for Event and EventTarget that exist in NodeJS now

Fyi, Node ships with experimental Blob support now now (v15.7) also, If you switch to ESM then you can use fetch-blob as a polyfill/fallback it only exports ESM (it ditched commonjs)

lpinca · 2021-07-05T06:16:33Z

@jimmywarting ab7b6d4 is about using Buffers instead of strings for text messages and for the close reason, not for binary messages. This is against the HTML specification where a DOMString is used and it is done for performance. For binary messages, users can already get an ArrayBuffer by setting the binaryType attribute to 'arraybuffer' as per HTML spec.

If you are planing on making a breaking change anyway, what do you feel about switching to ESM? node "^12.20.0 || ^14.13.0"

See #1886. I'm fine with that as long as it does not cause issues with bundlers.

While we are at it, go for Event and EventTarget that exist in NodeJS now

This requires Node.js >= 14.5.0 and comes with significant performance regressions. This is currently a no go.

Fyi, Node ships with experimental Blob support now now (v15.7) also

I know and it might make sense to change the default binaryType from 'nodebuffer' to 'blob' as per HTML spec but the feature is experimental and I don't want to use polyfills. I prefer to postpone this to the next major release.

lpinca · 2021-07-06T08:26:48Z

I really need feedbacks for 4edae51. If it is too breaking I will remove it.

cc: @cTn-dev @pimterry @netizen-ais @feross @mcollina

cTn-dev · 2021-07-06T09:22:47Z

This is definitely one hell of a breaking change.

While i am in favor of removing the "implicit" .toString() and let the user handle that logic (as already pointed out in some cases, it would be pretty invisible as far JSON.parse() is concerned.

Have the removal of the .toString() resulted in any significant performance improvement ?
I don't currently have an opinion on the isBinary argument (will have to think about it some more)

lpinca · 2021-07-06T09:30:33Z

Have the removal of the .toString() resulted in any significant performance improvement ?

Yes, especially when using the streaming interface. It prevents the data from being copied twice (from Buffer to string when a text message is received and back from string to Buffer when it is added to the internal queue of the Duplex) here https://github.com/websockets/ws/blob/7.5.2/lib/stream.js#L76.

Another similar situation is when sending a text message from a client to another:

wss.on('connection', function (ws) {
  ws.on('message', function (data, isBinary) {
    ws.send(data, { binary: isBinary});
  });
});

data is not copied twice in this case.

netizen-ais · 2021-07-06T10:08:40Z

My environment isn't big enough to get a hint about performance (2 ws servers with an average of 100 clients that connect to both node processes), but after testing this branch in the develop environment with no code changes on our base, I've tested it in one of the ws servers without any problem.

mcollina

I'm good with the changes!

mcollina · 2021-07-06T10:20:12Z

package.json

  "browser": "browser.js",
  "engines": {
-    "node": ">=8.3.0"
+    "node": ">=10.0.0"


I would recommend to make the jump to v12.

v10 is officially no longer supported but only two months have passed since its EOL date. I prefer to wait a little longer before dropping support for it, there is no additional maintenance burden compared to v12.

pimterry · 2021-07-06T16:28:54Z

Changes all make sense to me, definitely required quite a few updates on my end so it is a bit disruptive, but all very small and easy to do, and everything's still working nicely afterwards 👍

cTn-dev · 2021-07-08T08:31:34Z

I did a bit of testing against the 10f27d7 with both string and binary messages (haven't tried a mixing them in a single session).
Everything is working fine, the "migration" due to the breaking changes took < 5 minutes, as straight forward as it gets.

Make the `WebSocket` constructor throw a `SyntaxError` if any of the subprotocol names are invalid or duplicated.

Abort the handshake if the `Sec-WebSocket-Protocol` header is invalid.

Make the parser throw an error if the header field value is empty or if it begins or ends with a white space.

Make the `WebSocket` constructor throw a `SyntaxError` if the URL contains a fragment identifier or if the URL's protocol is not one of `'ws:'`, `'wss:'`, or `'ws+unix:'`.

Avoid decoding text messages and close reasons to strings. Pass them as `Buffer`s to the listeners of their respective events. Also, make listeners of the `'message'` event take a boolean argument to speficy whether or not the message is binary. Refs: #1878 Refs: #1804

Fixes #1886

jberger · 2021-07-12T20:58:21Z

I've just hit something that is obliquely related. I can open a new bug (and if it gets ignored here I will) but currently it seems to be impossible to write to a websocket stream as text. Any idea if you can make the writeableObjectMode useful while you make the readableObjectMode useful too?

(This affects me because the code that controls the websocket in the browser side assumes that it gets text and I have no control over that code https://github.com/glyptodon/guacamole-client/blob/754e9649f1fa0ba225ee42b56ded64bc283d17df/guacamole-common-js/src/main/webapp/modules/Tunnel.js#L1009)

It seems that a minimal fix is to simply allow the options to override the defaults that you give it https://github.com/websockets/ws/blob/master/lib/stream.js#L68 . I know that leaves the client open to breaking other options but hopefully "you break it, you bought it" applies.

lpinca · 2021-07-13T05:22:35Z

I can open a new bug (and if it gets ignored here I will) but currently it seems to be impossible to write to a websocket stream as text.

@jberger this is unrelated to this PR so yes, a new issue would have been better. Anyway you can use the decodeStrings option.

const WebSocket = require('ws');

const ws = new WebSocket(url);
const duplex = WebSocket.createWebSocketStream(ws, { decodeStrings: false });

duplex.write('foo');
duplex.write('bar');
duplex.write('baz');

lpinca · 2021-07-13T05:35:16Z

@pimterry the last two commits are based on our discussion in #1902. Can you please take a look when you have some time? Thank you.

pimterry · 2021-07-13T14:38:01Z

doc/ws.md

@@ -209,7 +209,8 @@ added when the `clientTracking` is truthy.

 Close the HTTP server if created internally, terminate all clients and call


This should probably say 'close' all clients now, not 'terminate'.

pimterry · 2021-07-13T14:47:00Z

lib/websocket-server.js

+        if (!this.clients.size) {
+          process.nextTick(emitClose, this);
+        } else {
+          for (const client of this.clients) client.close(1001);


From a networking POV, this all seems like a good improvement to me. It definitely seems clearer for clients if servers send a 1001 close frame and tidily shutdown connections where possible 👍

That said, it does mean the connections could now unexpectedly stay open whilst waiting for a close response, until the close timeout, if you have any poorly behaving clients. That might be problematic if users are trying to immediately shut everything down, which seems like a reasonable use case, and there's now no easy alternative way to do that when necessary.

Maybe the server should have a server.terminate() method too, which does terminate everything immediately?

That would make the server API match the individual websockets API, which already has websocket.terminate(), and it would cover that immediate-shutdown use case.

As a bonus, this could also easily support cases like "try to shut down cleanly, but then always close within X milliseconds" with just:

server.close(); setTimeout(() => server.terminate(), X);

Users can terminate clients by themself if they want to close immediately:

wss.close(); for (const ws of wss.clients) ws.terminate();

Ah ok, fair enough. I'm not sure if that trick will be obvious to people, so a convenience server.terminate() method to do the same could still be useful, but you're right that that means it's not strictly necessary 👍

It's probably worth mentioning this in the breaking-changes release notes though imo, as the suggested way to migrate for people who do need the previous behaviour.

I have two doubts:

If it is better to just prevent new connections from being established and not close/terminate existing clients. Closing clients is user responsibility so they can use websocket.close() or websocket.terminate() at will. This is also consistent with the behavior of server.close().

Regardless of 1., if/when to emit the 'close' event when client tracking is disabled. This is currently done in the next tick.

I went ahead and changed WebSocketServer.prototype.close() behavior in 6a238da. It no longer closes existing connections as per point 1. of my previous comment.

I've also updated the documentation for point 2. I think that emitting the 'close' event when connections are still active is a bit misleading but this behavior already exists and was not changed here.

Missed this sorry, I've been busy. That sounds very reasonable to me and the consistency with node is great.

I do think documenting this clearly for those who do want to terminate the server will be useful though, and it's not obvious right now. It might be worth a "how to fully shutdown a server" section in the readme, and/or the release notes.

When `WebSocketServer.prototype.close()` is called, stop accepting new connections but do not close the existing ones. If the HTTP/S server was created internally, then close it and emit the `'close'` event when it closes. Otherwise, if client tracking is enabled, then emit the `'close'` event when the number of connections goes down to zero. Otherwise, emit it in the next tick. Refs: #1902

Match the behavior of Node.js core `net.Server` and call the callback of `WebSocketServer.prototype.close()` with an error if the server is already closed.

lpinca force-pushed the breaking-changes branch 5 times, most recently from ae5362b to fba0658 Compare June 23, 2021 09:57

lpinca force-pushed the breaking-changes branch from fba0658 to 23eace8 Compare June 29, 2021 05:29

lpinca force-pushed the breaking-changes branch from a02969d to d8db7a0 Compare July 2, 2021 18:00

lpinca force-pushed the master branch from 431cc3e to aca94c8 Compare July 3, 2021 08:15

lpinca force-pushed the breaking-changes branch from d8db7a0 to ab7b6d4 Compare July 3, 2021 08:20

lpinca force-pushed the breaking-changes branch 5 times, most recently from b9eb026 to b60712d Compare July 6, 2021 08:10

mcollina reviewed Jul 6, 2021

View reviewed changes

lpinca force-pushed the breaking-changes branch from b60712d to 6e8ea4f Compare July 6, 2021 11:11

lpinca force-pushed the breaking-changes branch from 6e8ea4f to 10f27d7 Compare July 7, 2021 13:23

lpinca force-pushed the breaking-changes branch from 10f27d7 to d81eb6c Compare July 8, 2021 12:11

lpinca added 3 commits July 10, 2021 08:02

[major] Validate subprotocol names

0f5728d

Make the `WebSocket` constructor throw a `SyntaxError` if any of the subprotocol names are invalid or duplicated.

[major] Validate the Sec-WebSocket-Protocol header

d86fd1b

Abort the handshake if the `Sec-WebSocket-Protocol` header is invalid.

[major] Make the Sec-WebSocket-Extensions header parser stricter

49271dc

Make the parser throw an error if the header field value is empty or if it begins or ends with a white space.

lpinca added 4 commits July 10, 2021 08:02

[major] Drop support for Node.js < 10

9bba35c

[major] Throw an error if the connection URL is invalid

0031b55

Make the `WebSocket` constructor throw a `SyntaxError` if the URL contains a fragment identifier or if the URL's protocol is not one of `'ws:'`, `'wss:'`, or `'ws+unix:'`.

[major] Add an ES module wrapper

5c725ae

Fixes #1886

lpinca force-pushed the breaking-changes branch from d81eb6c to 5c725ae Compare July 10, 2021 06:02

lpinca force-pushed the breaking-changes branch 2 times, most recently from e98f3be to 09334ff Compare July 13, 2021 07:38

pimterry reviewed Jul 13, 2021

View reviewed changes

jberger mentioned this pull request Jul 13, 2021

Clarify use of text frames with streams #1915

Closed

lpinca force-pushed the breaking-changes branch 2 times, most recently from 2da7977 to c8615c4 Compare July 14, 2021 15:10

lpinca added 2 commits July 14, 2021 17:39

[major] Call the callback with an error if the server is closed

561bdaa

Match the behavior of Node.js core `net.Server` and call the callback of `WebSocketServer.prototype.close()` with an error if the server is already closed.

lpinca force-pushed the breaking-changes branch from c8615c4 to 561bdaa Compare July 14, 2021 15:39

lpinca merged commit abde9cf into master Jul 14, 2021

lpinca deleted the breaking-changes branch July 14, 2021 19:26

jonathanperret mentioned this pull request Mar 1, 2024

Support HTTP long polling as an alternative to WebSockets gristlabs/grist-core#859

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Breaking changes #1904

Breaking changes #1904

lpinca commented Jun 18, 2021 •

edited

Loading

lpinca commented Jul 2, 2021 •

edited

Loading

jimmywarting commented Jul 5, 2021 •

edited

Loading

lpinca commented Jul 5, 2021

lpinca commented Jul 6, 2021 •

edited

Loading

cTn-dev commented Jul 6, 2021

lpinca commented Jul 6, 2021 •

edited

Loading

netizen-ais commented Jul 6, 2021

mcollina left a comment

mcollina Jul 6, 2021

lpinca Jul 6, 2021 •

edited

Loading

pimterry commented Jul 6, 2021

cTn-dev commented Jul 8, 2021

jberger commented Jul 12, 2021 •

edited

Loading

lpinca commented Jul 13, 2021 •

edited

Loading

lpinca commented Jul 13, 2021

pimterry Jul 13, 2021

pimterry Jul 13, 2021

lpinca Jul 13, 2021 •

edited

Loading

pimterry Jul 13, 2021

lpinca Jul 13, 2021 •

edited

Loading

lpinca Jul 14, 2021 •

edited

Loading

pimterry Jul 15, 2021

		@@ -209,7 +209,8 @@ added when the `clientTracking` is truthy.

		Close the HTTP server if created internally, terminate all clients and call

Breaking changes #1904

Breaking changes #1904

Conversation

lpinca commented Jun 18, 2021 • edited Loading

lpinca commented Jul 2, 2021 • edited Loading

jimmywarting commented Jul 5, 2021 • edited Loading

lpinca commented Jul 5, 2021

lpinca commented Jul 6, 2021 • edited Loading

cTn-dev commented Jul 6, 2021

lpinca commented Jul 6, 2021 • edited Loading

netizen-ais commented Jul 6, 2021

mcollina left a comment

Choose a reason for hiding this comment

mcollina Jul 6, 2021

Choose a reason for hiding this comment

lpinca Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

pimterry commented Jul 6, 2021

cTn-dev commented Jul 8, 2021

jberger commented Jul 12, 2021 • edited Loading

lpinca commented Jul 13, 2021 • edited Loading

lpinca commented Jul 13, 2021

pimterry Jul 13, 2021

Choose a reason for hiding this comment

pimterry Jul 13, 2021

Choose a reason for hiding this comment

lpinca Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

pimterry Jul 13, 2021

Choose a reason for hiding this comment

lpinca Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

lpinca Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

pimterry Jul 15, 2021

Choose a reason for hiding this comment

lpinca commented Jun 18, 2021 •

edited

Loading

lpinca commented Jul 2, 2021 •

edited

Loading

jimmywarting commented Jul 5, 2021 •

edited

Loading

lpinca commented Jul 6, 2021 •

edited

Loading

lpinca commented Jul 6, 2021 •

edited

Loading

lpinca Jul 6, 2021 •

edited

Loading

jberger commented Jul 12, 2021 •

edited

Loading

lpinca commented Jul 13, 2021 •

edited

Loading

lpinca Jul 13, 2021 •

edited

Loading

lpinca Jul 13, 2021 •

edited

Loading

lpinca Jul 14, 2021 •

edited

Loading