http2 - compat request forward error

[ronag]

After thinking about this for a while I think we still should forward error events from the stream object. However, only if there is an event listener as to avoid uncaught errors.

function onStreamError(error) {
  const request = this[kRequest];
  if (request.hasListener('error')) {
    request.emit('error', error);
  }
}

Otherwise we end up with hacks like this:

 const incoming = req.stream || req
 incoming.on('error', errorHandler)

[benjamingr]

@nodejs/http2 @phouri and @apapirovski

[mcollina]

Ref: #14991

I am a bit lost, can you make a full example? I think the behavior right now is the same on both http and http2.

[ronag]

@mcollina: I don't see how/where the error would be forwarded to anything but the server object given https://github.com/nodejs/node/blob/master/lib/internal/http2/compat.js#L78

This is how it worked before, https://github.com/nodejs/node/blob/v8.4.0-proposal/lib/internal/http2/compat.js#L61, which forwarded the error on the request object but could lead to uncaught exception errors.

[mcollina]

@ronag I am still lost. Can you make an example (broken) on how it should behave and what you would expect? Can you show the same example using http so that I understand the difference?

[phouri]

Following

[ronag]

@mcollina

I'm not able to create a small enough repo.

This is the commit I'm talking about:

53c5bf5

Where this change was made:

 function onStreamError(error) {
 -  const request = this[kRequest];
 -  request.emit('error', error);
 +  // this is purposefully left blank
 +  //
 +  // errors in compatibility mode are
 +  // not forwarded to the request
 +  // and response objects. However,
 +  // they are forwarded to 'clientError'
 +  // on the server by Http2Stream
  }

Previously the error was forwarded through:

request.emit('error', error)

However, that was removed and if you read the comment it seems to imply that the error will only be forwarded to clientError.

errors in compatibility mode are not forwarded to the request and response objects

So if I want "correct" behavior I would have to do in userland:

(req.stream || req).on('error', onError)

[mcollina]

That behavior is lifted from require('http'). Part of the goal of the Compatibility API is to be compatible with require('http') so that the code handling the two would not be too different. Requests do not emit errors if the underlining API emits errors in http, so neither should http2.

The code linked has a typo, the correct event name is 'streamError':
https://nodejs.org/api/http2.html#http2_event_streamerror

(req.stream || req).on('error', onError)

You shouldn't need to to that at all. Why do you need to track down the error in this way? Why do you need to do that in http2 while you didn't need it in http?

Bear with us, require('http2') is flagged experimental and we can land semver-major changes between releases. Thank you very much to help ironing out all those quirks.

[ronag]

That behavior is lifted from require('http'). Part of the goal of the Compatibility API is to be compatible with require('http') so that the code handling the two would not be too different. Requests do not emit errors if the underlining API emits errors in http, so neither should http2.

Makes sense!

Bear with us, require('http2') is flagged experimental and we can land semver-major changes between releases. Thank you very much to help ironing out all those quirks.

No worries. Thank you for very patient attitude. Sorry for the issue spamming but I am also very keen on getting this working well.