read ECONNRESET error on disconnect (ws 3.3.3)

[G-Rath]

• I've searched for any related issues and avoided creating a duplicate issue.

Description

Upon updating to ws 3.3.3, when a client disconnects the following exception is thrown:

events.js:183
      throw er; // Unhandled 'error' event
      ^

Error: read ECONNRESET
    at _errnoException (util.js:1024:11)
    at TCP.onread (net.js:615:25)

Code works fine in previous version.

Reproducible in:

version: 3.3.3
Node.js version(s): v8.9.1
OS version(s): Windows 10 Pro, Fall update

Steps to reproduce:

1. Create a websocket server
2. Connect to the websocket server via Chrome (`new WebSocket('ws://localhost:3000'))
3. Disconnect from the websocket server (via refreshing the page in Chrome)

Expected result:

The disconnect is handled gracefully, with the 'close' event being called.

Actual result:

An exception is thrown, which crashes the app:

events.js:183
      throw er; // Unhandled 'error' event
      ^

Error: read ECONNRESET
    at _errnoException (util.js:1024:11)
    at TCP.onread (net.js:615:25)

[G-Rath]

Issue is caused by lack of error listener on the socket, and can be fixed like so:

ws.on('error', () => console.log('errored'));

This isn't documented in the README.md, and seems to be a pretty big breaking change.

[lpinca]

You need to always add a listener for the 'error' event as there are other errors that can be emitted.
net.Socket errors are no longer swallowed in version 3.3.3.

[wangjian1119]

his is the case for the latest Google browsers

[reverofevil]

@lpinca So can we add it to README? If it's a required thing to do, why is it absent there?

Edit. This is totally a good change, but it should be at least documented properly.

[lpinca]

It is documented, see https://github.com/websockets/ws/blob/180b388a2ed83fddd667d88970a943d0f47d32ec/doc/ws.md#event-error-1

[G-Rath]

@lpinca You document that that event is where any errors from underlying net.Socket are forwarded to, but not the fact that a simple client disconnection will cause such an exception.

To me, because you have a 'close' event, I'd assume that exceptions caused as a result of a client disconnecting would be handled by this library, since it's something that the library seems to be able to detect and act on.

I think is a reasonable assumption to make, given that clients disconnecting is a perfectly normal event for a socket server.

[quigleyj97]

We've been running into this issue as well, but it's puzzling.

Chrome 62 and below do not cause an error using these steps, and neither do any other browsers that I've tested (Firefox 57, Firefox Mobile (57), Chrome Mobile (62)).

Further, with Wireshark I wasn't able to observe any significant difference in the close frames. Both Firefox and Chrome send the same 1001 closing code ("Going away").

With the error handler, it appears when closing a Chrome 63 tab that the error handler and the close handler are called at roughly the same time, but I haven't yet confirmed this.

[lpinca]

@G-Rath

but not the fact that a simple client disconnection will cause such an exception.

Yes because it shouldn't, the issue seems to happen only on Chrome 63.

To me, because you have a 'close' event, I'd assume that exceptions caused as a result of a client disconnecting would be handled by this library, since it's something that the library seems to be able to detect and act on.

I think is a reasonable assumption to make, given that clients disconnecting is a perfectly normal event for a socket server.

Swallowing errors is never a good idea. Developers can ignore errors but they may want to know if the connection was closed cleanly or not.

@quigleyj97

I didn't check but maybe latest Chrome sends the close frame and abruptly closes the connection immediately after that.

[G-Rath]

@lpinca That all makes far more sense; that's a mistake on my part - I didn't test it with another browser, and so assumed that that was something that commonly happens with client disconnections.

[lpinca]

I can no longer reproduce the issue on Chrome 63.0.3239.108 can anyone confirm?

[lpinca]

Nevermind, here is a test case which doesn't use ws:

const assert = require('assert');
const crypto = require('crypto');
const http = require('http');

const GUID = '258EAFA5-E914-47DA-95CA-C5AB0DC85B11';

const data = `<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
  </head>
  <body>
    <script>
      (function () {
        var ws = new WebSocket('ws://localhost:8080');

        ws.onopen = function () {
          console.log('open');
        };
      })();
    </script>
  </body>
</html>`;

const server = http.createServer();

server.on('request', (req, res) => {
  res.setHeader('Content-Type', 'text/html');
  res.end(data);
});

server.on('upgrade', (req, socket) => {
  const key = crypto.createHash('sha1')
    .update(req.headers['sec-websocket-key'] + GUID)
    .digest('base64');

  socket.on('error', console.error);
  socket.on('data', (data) => {
    assert.ok(data.slice(0, 2).equals(Buffer.from([0x88, 0x82])));
    socket.write(Buffer.from([0x88, 0x00]));
  });

  socket.write([
    'HTTP/1.1 101 Switching Protocols',
    'Upgrade: websocket',
    'Connection: Upgrade',
    `Sec-WebSocket-Accept: ${key}`,
    '\r\n'
  ].join('\r\n'));
});

server.listen(8080, () => console.log('Listening on *:8080'));

[ProgrammingLife]

I'm in the same boat.

ws.on('error', () => console.log('errored'));

this doesn't solve the problem.

[lpinca]

@ProgrammingLife it should.
In #1266 (comment) you added the listener on the server so make sure you are adding it to the WebSocket instance instead.

[ICarryTheDustOfAJourney]

I'm in the same boat.

ws.on('error', () => console.log('errored'));

this doesn't solve the problem.

I can confirm this statement, when the handler is applied to the server. No more crashes when applied to each connection individually.

These errors occur quite often on Chrome V 63.0.3239.84 (=latest for my distribution).

FF 57.0.3 doesn't produce this error at all. Same with Edge on Windows clients.

Node v8.9.3 ws 3.3.3 ubuntu1~16.04.5 Linux 4.4.0-21-generic (x86_64)

[lpinca]

According to https://bugs.chromium.org/p/chromium/issues/detail?id=798194#c6 this works as intended.

What we can do here is adding a default 'error' listener which is a noop, so people are not forced to add one. Users can still get errors by adding their own 'error' listener(s).

Thoughts?

[EvanCarroll]

@kahluagenie here because of Devskiller interview assessment?

---

I wonder how many people are coming here because of bad spec tests in timed Devskiller assessments? Here is my theory: I think the ws team is getting played. =P Devskiller's package.json version-fixed to 3.3.3 which has the issue, and all we have three hours to figure out why we're too stupid to scaffold a Web Socket interface (beginner level stuff). The real reason is because they mirror in their .spec.ts a very subtle bug that's only valid on archaic versions of ws

wsClient.close();
server.close();

or,

ws.close()
wsServer.close()

You have three hours to scour and find the GitHub issue before you get the senior level remote-coffee-shop Node.JS guru position. If you found this issue. Congrats. You won.

The funny thing I read this and I wonder if everyone is making this honest mistake, or if we're all here for the same malformed spec/wsServer-api.spec.js. =P

https://devskiller.com/coding-tests/middle-javascript-developer-node-js-server-side-step-tracker/

[kahluagenie]

Haha, negative. I was using ws for a project of mine, so was figuring stuff out when I ran across the issue. All good now though!

[haroldiedema]

Apologies for commenting on an old issue, but this is still a problem.

Error: write EPIPE
    at afterWriteDispatched (internal/stream_base_commons.js:156:25)
    at writevGeneric (internal/stream_base_commons.js:139:3)
    at Socket._writeGeneric (net.js:783:11)
    at Socket._writev (net.js:792:8)
    at doWrite (internal/streams/writable.js:375:12)
    at clearBuffer (internal/streams/writable.js:521:5)
    at Socket.Writable.uncork (internal/streams/writable.js:317:7)
    at Sender.sendFrame (/home/harold/projects/mercury/server/mercury.js:34612:22)
    at Sender.send (/home/harold/projects/mercury/server/mercury.js:34507:12)
    at WebSocket.send (/home/harold/projects/mercury/server/mercury.js:35382:18)

Whenever a client disconnects while a large amount of data is being sent, I consistently get full server crashes.
Everywhere in my code, I've added listeners to the error event where possible; on the http server, request and response object, the socket server and each individual client. I've even gone as far as ws._socket.on('error', () => ...), but to no avail.

Using ws 7.4.2, running on Node v14.15.4 on Ubuntu 20.04.

I'm actually unsure if this is a node internal error or has something to do with this library, since I also get ECONNRESET errors that I can't seem to catch from time to time.

edit:

Right as I finished up writing this post, I figured the listener was attached too late to the error event. My server runs inside a worker thread and the issue only seems to occur when my CPU load is reasonably high. The crash happened when the HTTP-server connection event is fired, then the client disconnects, and then the request should be fired. It's not a problem with this library, but the fact the error event is emitted from the socket before an event listener is attached to it.

For anyone running into similar issues: The fix for me was to attach a listener to the error event immediately as the client connects, and not a 'tick' later, because the client disconnects after the connection event, but before the upgrade event.

So, instead of:

httpServer.on('upgrade', (request: IncomingMessage, socket: Socket, head: Buffer) => {
    socket.on('error', () => console.log('poof!'));
});

All you have to do is bind the listener during the connection event.

httpServer.on('connection', (socket: Socket) => {
    socket.on('error', () => console.log('poof!'));
});

[cluxter]

@haroldiedema Could you please provide us with a full proof of concept that makes your server crash?

[haroldiedema]

@cluxter Sorry, I don't think I can. It was triggered by a combination of circumstances (high CPU load, multiple active connections, etc.) The problem was on my end.

All I had to do was immediately attach a listener to the error event on the socket when the connection event got fired on the http server (from nodejs itself). Most documentation / similar reports I've seen regarding this issue all say the same: Add an error listener to the client, but nobody explicitly mentions to do it during the connection event.

The problem specifically is when the client connects (page is loading), and before the HTTP server request event got fired, the client disconnects, due to a page reload or closing of the browser tab. This consistently crashes the server because the request event wasn't fired yet, so no listener was attached to the error event 😛

Anyway, like I said, it's not a problem with the 'ws' library. People running into this issue should simply attach the error listener immediately when the client connects, and not a bit later to prevent these kinds of 'race conditions'.

[lpinca]

@haroldiedema that listener is already added by Node.js as per https://github.com/nodejs/node/blob/v15.6.0/lib/_http_server.js#L498.