spdy-http2 / node-spdy

SPDY server on Node.js
2.81k stars 196 forks source link

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

Open bgrieder opened 7 years ago

bgrieder commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 */ } )
})