Detecting broken connections fails with `RSV1 must be clear` on node v20.11.0 about ws HOT 14 CLOSED

On second look, it seems that the else branch is never taken. I think wss.handleUpgrade() is never called with the above code.

What do you mean? The websocket handshake is done, and wscat does output the periodically send timestamp.

You can run it on your own. Its a minimal reproducible example.

from ws.

Same happens with module version v8.10 on node 20.11.0

When multiple connections are made to the server, they do not drop all at the same time, but more after they are done. E.g:

• 1fst connection is done
• 30 sec delay
• 2snd connection is done
• some time pass
• 1st connection is dropped/errores
• 30 sec passes
• 2nd connection errorres out

from ws.

The error you are seeing happens when a client sends an invalid frame. See #2169. Also this snippet

app.get('/', (req, res) => {
  if (!req.headers['upgrade'] || !req.headers['connection']) {
    // no websocket handshake
    res.status(421).end();
  } else {
    wss.handleUpgrade(req, req.socket, req.headers, (ws) => {
      ws.isAlive = true;

      ws.on('pong', () => {
        ws.isAlive = true;
      });

      wss.emit('connection', ws, req);
    });
  }
});

is wrong. app.get() does not handle the 'upgrade' event of the server. It handles the 'request' event. This means that you are calling wss.handleUpgrade() with the socket of a normal (non-upgrade) request which is probably why you see that error.

from ws.

On second look, it seems that the else branch is never taken. I think wss.handleUpgrade() is never called with the above code.

from ws.

Thanks for the response. But what i do not understand, why this just happens after a update from node 16 to 20. In v16 everything works fine and like expected. Why is this not the case after a update to v20?

he error you are seeing happens when a client sends an invalid frame

This happens the other way around. Seems like the server sends a invalid frame.
Without this snippet, it works:

// PROBLEM START HERE
let interval = setInterval(() => {
    wss.clients.forEach((ws) => {

        if (!ws.isAlive) {
            ws.terminate();
            return;
        }

        ws.isAlive = false;
        ws.ping();

    });
}, 5000);


wss.on("close", () => {
    clearInterval(interval);
});
// PROBLEM ENDS HERE

For the upgrade handling part, i never had any issues doing it this way.
req.socket socket is the underlying net.Socket used for the http request, and nothing else is send over the tcp socket but the websocket. And it worked like a charm in v16.

I used the approach with express because i have multiple websocket endpoints and like the url/http request handling that express provides.

Regardless how the upgrade is done, why does it (not) work when the broken connection detection part is (un-)commented? The error only happens when ping events are send and node =<16. At nearly exact after ~80 seconds.

Makes no sense to me.
I dont want to bother you, just try to understand it better.

from ws.

You are right with the part how i handle the websocket upgrade.
When i listen for the upgrade event, it works too in newer node versions.

What i find interesting is, that, when i add to my existing code (found here: expressjs/express#2594 (comment)):

server.on("upgrade", (req, socket, head) => {

    let res = new http.ServerResponse(req);
    res.assignSocket(socket)

    res.on("finish", () => {
        res.socket.destroy();
    });

    app(req, res);

});

Without changing anything else, the problem is resolved and it works.
What fascinates me, is the fact, that i use in the express route handler still:

        wss.handleUpgrade(req, req.socket, req.headers, (ws) => {

            ws.isAlive = true;

            ws.on("pong", () => {
                ws.isAlive = true;
            });

            wss.emit("connection", ws, req);

        });

What/Why is req.socket something different if handled by "upgrade" or "request" event.
They refer to the same underlying tcp/net.Socket, even in the docs they say that.
And as said in earlier comments, it worked till node version 18/20.

from ws.

What/Why is req.socket something different if handled by "upgrade" or "request" event.

Because the socket is freed when the 'upgrade' event is emitted. The 'request' handler keeps all the HTTP machinery. In older versions of Node.js the 'request' event was not emitted for upgrade requests. It seems that now it is emitted when there is no 'upgrade' listener.

from ws.

Do you see any downsites how i handle the upgrade/routing stuff this way?

from ws.

Keep all the WebSocket stuff in the 'upgrade' handler. Once you add it the express route handler will no longer be used for upgrade requests.

from ws.

The error you are seeing is probably caused by some spurious HTTP stuff written to the raw socket used by the WebSocket.

from ws.

Makes somehow sense, but why "exactly" after ~1:20 and only when the ping events from How to detect and close broken connections? are used?

from ws.

Maybe your issue started after this change nodejs/node#43522.

from ws.

Makes somehow sense, but why "exactly" after ~1:20 and only when the ping events from ...

I don't know, probably because there are other open sockets. ws.terminate() destroys the socket, but there might be other sockets open for non upgrade requests.

from ws.

Thank you for helping. I close this issue. its resolved for me and not a problem of the ws package :)

from ws.