Added error handling section to readme #140
Conversation
|
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. |
|
@mcollina is right. The example you'd like to add to the docs doesn't cover all cases, e.g. with no any |
|
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 |
|
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. |
| socket.on('error', function(err) { | ||
| console.log("Client socket error:", err); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This is the block that should not be needed, not the other one. Why is this needed?
|
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 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In readme.md
<#140 (comment)>
:
> @@ -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
<#140 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHfgJFlZ3XDYlMVNQjz1KbxHKF50x5Aks5tadSRgaJpZM4SOq_y>
.
|
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.