Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch or prevent errors to be emitted on streams when a socket closes unexpectedly #282

Open
bgrieder opened this issue Sep 29, 2016 · 3 comments

Comments

@bgrieder
Copy link

When a socket closes (unexpectedly), the 'close' event triggers on the Connection object, which issues a call to its method detroyStreams(), that emits errors on all the underlying streams which are still active.

This is OK in principle but the problem is that failing to register an 'error' listener on the streams, causes havoc in the app trough an unchecked exception.

In other words, there is no way of gracefully handling a socket closing unexpectedly unless there is a way of either preventing the errors to be emitted or providing a way of registering error listeners on the streams.

Could you please look into this ?

In connection.js

this.socket.once('close', function onclose() {
    ...
   self.destroyStreams(err);
   ...
})

Connection.prototype.destroyStreams = function destroyStreams(err) {
  var state = this._spdyState;
  Object.keys(state.stream.map).forEach(function(id) {
    var stream = state.stream.map[id];

    stream.abort();
    stream.emit('error', err);   <--- will cause havoc in the app if no listener is registered
  });
};
@indutny
Copy link
Collaborator

indutny commented Sep 29, 2016

@bgrieder thank you for reporting this! Do you suggest that stream should not throw errors after .abort()?

I believe that it is quite common practice in node.js apps to listen for error events on streams. Technically, error may be emitted in many other cases, not just on the socket close.

@indutny
Copy link
Collaborator

indutny commented Sep 29, 2016

Oh, that code quote was from spdy-transport itself. So I guess you was talking about error events in general. If this is the case - I can only suggest to listen for them, this will save you from the trouble anyway.

@bgrieder
Copy link
Author

bgrieder commented Sep 30, 2016

@indutny Yes I am happy with listening to error events. The problem is that the streams are not accessible from the public API of node-spdy so I cannot hook a listener to them (they are attached to the private _spdyState property of the Connection)

Can I suggest that everytime a new stream is created the Connection instance emits a newStream event with the stream as a parameter ?

I could then do something like

conx.on('newStream', (stream) => {
    stream.once('error', () => { /* handle stream error gracefully */ } )
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants