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

[v9.x backport] http2: fixes error handling #19478

Closed

Conversation

mcollina
Copy link
Member

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]

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

@mcollina mcollina requested a review from MylesBorins March 20, 2018 14:34
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Mar 20, 2018
@mcollina
Copy link
Member Author

mcollina commented Mar 20, 2018

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

edit by @MylesBorins: looks like we had a race condition. Kicked off CI and updated link

@mcollina mcollina requested a review from jasnell March 20, 2018 14:36
@mcollina mcollina mentioned this pull request Mar 20, 2018
3 tasks
@MylesBorins
Copy link
Contributor

@mcollina seems like quite a few failures on this one. Want to try rebasing and running CI again?

@MylesBorins MylesBorins reopened this 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 mcollina force-pushed the backport-19232-to-v9.x-ter branch from 2a0aa23 to f36e5b4 Compare March 21, 2018 08:04
@mcollina
Copy link
Member Author

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]>
@MylesBorins
Copy link
Contributor

landed in 3914e97

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.

3 participants