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

http2: refactor error handling #14991

Closed
wants to merge 2 commits into from
Closed

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Aug 23, 2017

This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'clientError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing

See: #14963

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

http2

@mcollina mcollina requested a review from jasnell August 23, 2017 16:35
@nodejs-github-bot nodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label Aug 23, 2017
@mcollina
Copy link
Member Author

cc @nodejs/http2 @akc42

@@ -58,8 +58,12 @@ function onStreamEnd() {
}

function onStreamError(error) {
const request = this[kRequest];
request.emit('error', error);
// this is purpusefully left blank
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/purpusefully/purposefully

const server = session[kServer];

if (err && server) {
server.emit('clientError', err, this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: should this be nextTick'd?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so - or at least I'd expect it to. Do we have a guarantee that it will always be deferred by the time _destroy is called?

Copy link
Member Author

@mcollina mcollina Aug 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a nextTick on top, https://github.com/nodejs/node/blob/master/lib/internal/http2/core.js#L1509

_destroy is called synchronously. But the error of the _destroy callback will be emitted asynchronously, see https://github.com/nodejs/node/blob/master/lib/internal/streams/destroy.js#L32-L37.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but it may be good to add some detail to the documentation.

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with 2 nits - would love docs for the newly exposed Http2Stream and Http2Session

lib/http2.js Outdated
@@ -28,4 +30,6 @@ module.exports = {
connect,
Http2ServerResponse,
Http2ServerRequest,
Http2Session,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this - but increasing the surface of the API is not trivial - and I'd like to see docs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added them just for test purpose. I'll require the internals.

const server = session[kServer];

if (err && server) {
server.emit('clientError', err, this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so - or at least I'd expect it to. Do we have a guarantee that it will always be deferred by the time _destroy is called?

@akc42
Copy link

akc42 commented Aug 24, 2017

The two use cases I have been battling with is

  1. Client sends a last ditch "release_locks" api call on window unload, but has gone away before that api call completes and calls response.end(). I want to catch that situation and log that it has occurred - not have the server crash. But this is distinctly different from ...

  2. When client requests a static file that doesn't exist. This might be because the file genuinely doesn't exist (and therefore I send a 404), or because the client has tried to open a client side route which I need to respond with index.html. I don't really want to be opening a file just to check that prior to using respondWithFile. From a compat layer point of view, it might be better to hoist the core function on the ServerHttp2Stream to the ServerHttp2Response and have that error on this.

@mcollina mcollina force-pushed the http2-errors branch 2 times, most recently from 22498dd to 4d49f2a Compare August 24, 2017 10:16
@mcollina
Copy link
Member Author

I have moved Http2Stream from emitting 'clientError' to emitting 'streamError', because 'clientError' has a specific semantics on HTTP1, and we cannot provide that semantic with HTTP2.

@mcollina
Copy link
Member Author

I've added a commit that introduces onError for respondWithFile, that should allow to handle 404 or redirects correctly (cc @akc42).

@jasnell @benjamingr please reaffirm your LGTM. It has changed enough to be worth a fresh review.

@mcollina
Copy link
Member Author

@mcollina
Copy link
Member Author

This would be a semver-major change if we were not experimental.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I likes it!

Trott pushed a commit to Trott/io.js that referenced this pull request Aug 25, 2017
This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing

See: nodejs#14963

PR-URL: nodejs#14991
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@jasnell
Copy link
Member

jasnell commented Aug 25, 2017

Landed in 4ca8ff2

@jasnell jasnell closed this Aug 25, 2017
@Trott
Copy link
Member

Trott commented Aug 26, 2017

Sorry to report: This has broken CI.

By all appearances, this should never have landed in this condition. The CI was not green. One of the tests added failed on all Windows variants.

@Trott
Copy link
Member

Trott commented Aug 26, 2017

Windows test failure with this:

https://ci.nodejs.org/job/node-test-binary-windows/10729/RUN_SUBSET=1,VS_VERSION=vcbt2015,label=win10/console

not ok 202 parallel/test-http2-respond-file-404
  ---
  duration_ms: 0.170
  severity: fail
  stack: |-
    (node:4708) ExperimentalWarning: The http2 module is an experimental API.
    assert.js:42
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 'ENOENT: no such file or directory, open \'c:\\workspace\\node-test-binary-windows\\RUN_SUBSET\\1\\VS_VERSION\\vcbt2015\\label\\ === 'ENOENT: no such file or directory, open \'./not-a-file\''
        at c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vcbt2015\label\win10\test\common\index.js:715:16
        at c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vcbt2015\label\win10\test\common\index.js:509:15
        at onError (c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vcbt2015\label\win10\test\parallel\test-http2-respond-file-404.js:25:9)
        at ServerHttp2Stream.afterOpen (internal/http2/core.js:1704:7)
        at FSReqWrap.oncomplete (fs.js:135:15)

@Trott
Copy link
Member

Trott commented Aug 26, 2017

Reverted, so reopening so the test can be fixed and this can hopefully be re-landed soon.

@Trott Trott reopened this Aug 26, 2017
This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing

See: nodejs#14963
@mcollina
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/9851/

(should be ok this time, please verify)

cc @jasnell

@mcollina
Copy link
Member Author

Landed as 53c5bf5.

@mcollina mcollina closed this Aug 27, 2017
mcollina added a commit that referenced this pull request Aug 27, 2017
This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing.
It also updates respondWith* to the new error handling.

Fixes: #14963
PR-URL: #14991
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing

See: nodejs/node#14963

PR-URL: nodejs/node#14991
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing.
It also updates respondWith* to the new error handling.

Fixes: nodejs/node#14963
PR-URL: nodejs/node#14991
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing

See: nodejs/node#14963

PR-URL: nodejs/node#14991
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing.
It also updates respondWith* to the new error handling.

Fixes: nodejs/node#14963
PR-URL: nodejs/node#14991
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing.
It also updates respondWith* to the new error handling.

Fixes: #14963
PR-URL: #14991
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing.
It also updates respondWith* to the new error handling.

Fixes: #14963
PR-URL: #14991
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@mcollina mcollina mentioned this pull request Mar 8, 2018
3 tasks
mcollina added a commit to mcollina/node that referenced this pull request Mar 8, 2018
There should be no default error handling when using Http2Stream.
All errors will end up in `'streamError'` on the server anyway,
but they are emitted on `'stream'` as well, otherwise some error
conditions are impossible to debug.

See: nodejs#14991
mcollina added a commit that referenced this pull request Mar 12, 2018
There should be no default error handling when using Http2Stream.
All errors will end up in `'streamError'` on the server anyway,
but they are emitted on `'stream'` as well, otherwise some error
conditions are impossible to debug.

See: #14991

PR-URL: #19232
Reviewed-By: James M Snell <[email protected]>
mcollina added a commit to mcollina/node that referenced this pull request Mar 20, 2018
There should be no default error handling when using Http2Stream.
All errors will end up in `'streamError'` on the server anyway,
but they are emitted on `'stream'` as well, otherwise some error
conditions are impossible to debug.

See: nodejs#14991

PR-URL: nodejs#19232
Reviewed-By: James M Snell <[email protected]>
mcollina added a commit to mcollina/node that referenced this pull request Mar 21, 2018
There should be no default error handling when using Http2Stream.
All errors will end up in `'streamError'` on the server anyway,
but they are emitted on `'stream'` as well, otherwise some error
conditions are impossible to debug.

See: nodejs#14991

PR-URL: nodejs#19232
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2018
There should be no default error handling when using Http2Stream.
All errors will end up in `'streamError'` on the server anyway,
but they are emitted on `'stream'` as well, otherwise some error
conditions are impossible to debug.

See: #14991

Backport-PR-URL: #19478
PR-URL: #19232
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
There should be no default error handling when using Http2Stream.
All errors will end up in `'streamError'` on the server anyway,
but they are emitted on `'stream'` as well, otherwise some error
conditions are impossible to debug.

See: nodejs#14991

PR-URL: nodejs#19232
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants