http2: add missing error handlers #19986

ryzokuken · 2018-04-12T20:07:41Z

Attach simple error handlers in http2/core.js on sockets where they
seem to be missing to avoid the process from crashing.

Fixes: #19978

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

cc @mafintosh @nodejs/http2

Attach simple error handlers in http2/core.js on sockets where they seem to be missing to avoid the process from crashing. Fixes: nodejs#19978

jasnell · 2018-04-12T20:58:43Z

lib/internal/http2/core.js

@@ -868,6 +868,7 @@ class Http2Session extends EventEmitter {

    if (!socket._handle || !socket._handle._externalStream) {
      socket = new StreamWrap(socket);
+      socket.on('error', () => {});


I don't think we should be swallowing the error on these. Those should be forwarded on so they can be properly handled.

@jasnell agreed. Should I throw something descriptive inside the function block? Just forwarding the same error further seems counterintuitive.

Perhaps something that'd make the failure (and why exactly it happened) more obvious?

@mafintosh and @mcollina ... what do you think?

I think the error should be forwarded to session.

@mcollina

by session, I suppose you mean the current instance of Http2Session itself, right? I inferred that from socket[kSession] = this

If that's the case, I hope the following code does what you expected:

if (!socket._handle || !socket._handle._externalStream) { socket = new StreamWrap(socket); - socket.on('error', () => {}); + socket.on('error', (err) => { + this.emit('error', err); + }); }

mcollina

Can you add a couple of unit tests for this behavior?

mcollina · 2018-04-14T10:58:27Z

lib/internal/http2/core.js

@@ -2526,6 +2527,8 @@ function connectionListener(socket) {
                 'If this is a HTTP request: The server was not ' +
                 'configured with the `allowHTTP1` option or a ' +
                 'listener for the `unknownProtocol` event.\n');
+
+      socket.on('error', () => {});


Can you move this to before the call to end()?

mcollina · 2018-04-14T11:00:25Z

lib/internal/http2/core.js

@@ -868,6 +868,7 @@ class Http2Session extends EventEmitter {

    if (!socket._handle || !socket._handle._externalStream) {
      socket = new StreamWrap(socket);
+      socket.on('error', () => {});


I think the error should be forwarded to session.

apapirovski · 2018-04-14T11:03:23Z

lib/internal/http2/core.js

@@ -2526,6 +2527,8 @@ function connectionListener(socket) {
                 'If this is a HTTP request: The server was not ' +
                 'configured with the `allowHTTP1` option or a ' +
                 'listener for the `unknownProtocol` event.\n');
+
+      socket.on('error', () => {});


In addition to the other feedback, could we abstract this into const noop = () => {}; somewhere at the top level? Perhaps close to the emit helper.

ryzokuken · 2018-04-14T15:12:15Z

@apapirovski @mcollina I hope this looks good?

@mcollina I will be adding the required tests by tonight.

ryzokuken · 2018-04-14T19:07:58Z

@mcollina lib/http2.js contains:

'use strict';

process.emitWarning(
  'The http2 module is an experimental API.',
  'ExperimentalWarning', undefined,
  'See https://github.com/nodejs/http2'
);

const {
  constants,
  getDefaultSettings,
  getPackedSettings,
  getUnpackedSettings,
  createServer,
  createSecureServer,
  connect,
  Http2ServerRequest,
  Http2ServerResponse
} = require('internal/http2/core');

module.exports = {
  constants,
  getDefaultSettings,
  getPackedSettings,
  getUnpackedSettings,
  createServer,
  createSecureServer,
  connect,
  Http2ServerResponse,
  Http2ServerRequest
};

My point here: it neither exports Http2Session nor Http2Stream. How am I supposed to test them?

Should I export the two, or should I try to trigger the above failures and errors using the currently exported createServer and the likes?

ryzokuken · 2018-04-14T19:17:15Z

Also, in core.js, it is evident that the constructor for Http2Session takes three arguments:

...
class Http2Session extends EventEmitter {
  constructor(type, options, socket) {
    super();
...

However, in the docs

node/doc/api/http2.md

Line 92 in 6376d43

### Class: Http2Session

not only are the constructor parameters not mentioned anywhere, it would be really helpful if we listed out the valid values a variable like type can hold.

Perhaps I could look for more stuff than just missing event arguments as a part of nodejs/help#877?

apapirovski

This is on its way but I don't think the current behaviour is quite correct.

apapirovski · 2018-04-16T07:39:59Z

lib/internal/http2/core.js

@@ -868,6 +870,9 @@ class Http2Session extends EventEmitter {

    if (!socket._handle || !socket._handle._externalStream) {
      socket = new StreamWrap(socket);
+      socket.on('error', (err) => {


I'm not sure what the proper error handling would look like here. I think it might need to do more than emit and it should also be aware of whether the socketOnError already fired previously.

Maybe something like this would be more accurate:

const wrappedSocket = socket; socket = new StreamWrap(wrappedSocket); socket.on('error', socketOnError.bind(wrappedSocket));

This would avoid the JSStreamWrap propagating an already handled error from the underlying socket and emitting on the session twice.

/cc @jasnell @mcollina @addaleax

I also think this might need to handle socket.on('close') in a similar manner. So something like:

socket.on('close', socketOnClose.bind(wrappedSocket));

Proper error handling here is a simple .on('error', () => {}) as the underlying stream is already handled before hitting this switch

/cc @ryzokuken

Proper error handling here is a simple .on('error', () => {}) as the underlying stream is already handled before hitting this switch

That presumes that the wrap can't error itself...

(Which it can for example on line 56 in the JSStreamWrap itself, not to mention the fact that it inherits from Socket so there are likely other errors possible too.)

I would like input from @mcollina and @jasnell if possible. I think the following is correct:

const wrappedSocket = socket; socket = new StreamWrap(wrappedSocket); socket.on('error', socketOnError.bind(wrappedSocket)); socket.on('close', socketOnClose.bind(wrappedSocket));

that will trigger the error twice, as the wrappedSocket already triggers socketOnClose and socketOnError.

I think it would be fine to trigger an error if the wrappedSocket is in objectMode immediately as that seems to be the only case where the streamwrap emits a non-forwarded error? And then socket.on('error', () => {}) the rest, as they are already handled

Looking at the http2 code it seems ok to call socketOnError more than once as that simply calls destroy which should guard against that. The onClose method has a bit more going on so can't tell if that is safe to do there, but i don't think we need to forward that in either case?

so

const wrappedSocket = socket; socket = new StreamWrap(wrappedSocket); socket.on('error', socketOnError.bind(wrappedSocket));

?

that will trigger the error twice, as the wrappedSocket already triggers socketOnClose and socketOnError.

It will just bail because socketOnError removes the session assignment in a sync call. I'm also not sure that the error in objectMode is the only one. The wrap is a socket in itself, it can throw other related errors.

And I think you're probably right re: leaving out close listener. So the thing posted above seems good to me.

apapirovski · 2018-04-16T07:47:44Z

ping @addaleax re: best ways to test the error handling on JSStreamWrap. Any ideas? (In ways that don't trigger the error on the underlying socket instead.)

apapirovski · 2018-04-16T08:16:47Z

not only are the constructor parameters not mentioned anywhere, it would be really helpful if we listed out the valid values a variable like type can hold.

If it's not a part of the public API so we shouldn't really expose that type of information since it wouldn't be of any use and could be deceptive regarding its usage.

Should I export the two, or should I try to trigger the above failures and errors using the currently exported createServer and the likes?

For the unknown protocol one, we could create a straightforward server, try to connect using http1 (via https) and do something like:

server.on('connection', (socket) => {
  socket.emit('error', new Error('connection error'));
});

then that either blows it up or doesn't.

mafintosh · 2018-04-16T08:34:28Z

To test the streamwrap make a test that uses the createConnection api to return a stream that fails on the first write

ryzokuken · 2018-04-18T20:49:22Z

ping @addaleax

BridgeAR · 2018-05-15T15:26:15Z

@nodejs/http2 @nodejs/streams @addaleax PTAL

mcollina · 2018-05-16T19:55:57Z

This is waiting for a unit test to be written, see #19986 (comment) as a suggestion.

ryzokuken · 2018-05-24T04:35:09Z

@mcollina sorry that I totally forgot about this, will start working on the test.

jasnell · 2018-08-10T17:41:43Z

Ping @ryzokuken

ryzokuken · 2018-08-12T10:58:35Z

@jasnell I have lost a lot of context on this, but IIRC, there were a few disagreements regarding the nature of tests. I'll hit up @mafintosh over the course of the next week and try to get this one in, thanks!

jasnell · 2018-09-10T18:11:10Z

Ping again :-)

antsmartian · 2019-01-15T08:42:16Z

Ping pong @ryzokuken :)

BridgeAR · 2019-03-09T23:44:16Z

Closing due to a very long inactivity. @ryzokuken please reopen or open a new PR in case you want to continue working on this.

http2: add missing error handlers

ac893b6

Attach simple error handlers in http2/core.js on sockets where they seem to be missing to avoid the process from crashing. Fixes: nodejs#19978

nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Apr 12, 2018

jasnell reviewed Apr 12, 2018

View reviewed changes

mcollina reviewed Apr 14, 2018

View reviewed changes

apapirovski reviewed Apr 14, 2018

View reviewed changes

Incorporate feedback

cd4af2a

apapirovski reviewed Apr 16, 2018

View reviewed changes

Trott removed the dont-land-on-v4.x label Jul 19, 2018

jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018

BridgeAR closed this Mar 9, 2019

http2: add missing error handlers #19986

http2: add missing error handlers #19986

Conversation

ryzokuken commented Apr 12, 2018

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented Apr 14, 2018 • edited Loading

ryzokuken commented Apr 14, 2018 • edited Loading

ryzokuken commented Apr 14, 2018 • edited Loading

apapirovski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mafintosh Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Apr 16, 2018

apapirovski commented Apr 16, 2018 • edited Loading

mafintosh commented Apr 16, 2018

ryzokuken commented Apr 18, 2018

BridgeAR commented May 15, 2018

mcollina commented May 16, 2018

ryzokuken commented May 24, 2018

jasnell commented Aug 10, 2018

ryzokuken commented Aug 12, 2018

jasnell commented Sep 10, 2018

antsmartian commented Jan 15, 2019

BridgeAR commented Mar 9, 2019

ryzokuken commented Apr 14, 2018 •

edited

Loading

ryzokuken commented Apr 14, 2018 •

edited

Loading

ryzokuken commented Apr 14, 2018 •

edited

Loading

apapirovski Apr 16, 2018 •

edited

Loading

mafintosh Apr 19, 2018 •

edited

Loading

apapirovski commented Apr 16, 2018 •

edited

Loading