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.