Added error handling section to readme

[Juul]

It isn't obvious that not having an error handler on the stream itself will cause problems down the line. Intuitively one might expect that handling errors on the underlying TCP socket would be enough but that's not the case. It took me a while to track down this problem in my own code so I thought I'd document and save others the trouble.

[mcollina]

Instead of fixing the documentation, can you provide a fix for the actual problem? I think we should not crash the application in that case.

[DmitryMyadzelets]

@mcollina is right. The example you'd like to add to the docs doesn't cover all cases, e.g. with no any http server. This module encapsulates the ws module, hence it should take care of it.

[Juul]

Alright if we don't want to crash because of an unhandled error, even when the user does not attach an error handler to the stream, then I don't see any way out of attaching a no-op error handler. Pull request #141 implements this.

I worry that my readme pull request caused some confusion: Attaching an error handler to the http server is not necessary. I wanted to show that even if attaching error handlers to the socket beneath the stream at all possible layers the error is not considered handled. Attaching a single error handler to the stream passed to the createServer callback is always enough in and of itself.

[mcollina]

I mean a completely different fix. Not just ignoring the error, but routing it to our websocket-stream, so only one error handler can be added.

[Juul]

You are correct. I was trying to point out in the readme that even if you alrady have an error handler at a lower network layer, e.g. TCP or HTTP, then you still need an error handler for the stream. Otherwise you get the confusing result that e.g. an ECONNRESET triggers your error handler on the socket but your program still crashes due to an unhandled error. I was under the impression that this is the expected behaviour but becomes confusing if e.g. re-using the same server socket for multiple things when mixing normal http and websocket-stream

On Fri, Mar 2, 2018 at 3:24 PM, Matteo Collina notifications@github.com wrote:

@mcollina commented on this pull request.

In readme.md https://github.com/maxogden/websocket-stream/pull/140#discussion_r171988265 :

@@ -129,6 +129,31 @@ app.ws('/bigdata.json', function(ws, req) { app.listen(3000);

+### Error handling + +Errors both from the underlying websocket and from the TCP socket itself are propagated and emitted on the stream so you must provide an error handler for the stream even if you already have an error handler on the websocket, e.g: + +```javascript +var websocket = require('websocket-stream') +var wss = websocket.createServer({server: someHTTPServer}, handle) + +// this is not enough! +someHTTPServer.on('connection', function(socket) {

• socket.on('error', function(err) {
• console.log("Client socket error:", err);
• }); +});

This is the block that should not be needed, not the other one. Why is this needed?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/maxogden/websocket-stream/pull/140#pullrequestreview-100943994, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHfgJFlZ3XDYlMVNQjz1KbxHKF50x5Aks5tadSRgaJpZM4SOq_y .