Skip to content

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

@nodejs-github-botnodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Mar 20, 2018
@mcollina
Copy link
MemberAuthor

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

@mcollinamcollina requested a review from jasnellMarch 20, 2018 14:36
@mcollinamcollina 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?

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]>
@mcollinamcollinaforce-pushed the backport-19232-to-v9.x-ter branch from 2a0aa23 to f36e5b4CompareMarch 21, 2018 08:04
@mcollina
Copy link
MemberAuthor

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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http2Issues or PRs related to the http2 subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@mcollina@MylesBorins@nodejs-github-bot