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: don't send trailers on a closed connection #23146

Closed
wants to merge 1 commit into from

Conversation

edevil
Copy link
Contributor

@edevil edevil commented Sep 28, 2018

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

Addresses koajs/koa#1229 and #22135.

There is a race condition between onStreamCloseResponse(), which
removes the wantTrailers listener, and Http2Stream.close(), which
will invalidate the connection. IE, sendTrailers can be called on
a closed connection which would crash with a:
Error [ERR_HTTP2_INVALID_STREAM]: The stream has been destroyed
@nodejs-github-bot nodejs-github-bot added dont-land-on-v6.x http2 Issues or PRs related to the http2 subsystem. labels Sep 28, 2018
@trivikr
Copy link
Member

trivikr commented Sep 29, 2018

@edevil
Copy link
Contributor Author

edevil commented Sep 29, 2018

@trivikr I’m not familiar with node’s CI. Are those failures flakes, or can they be related to this PR? Thanks.

@trivikr
Copy link
Member

trivikr commented Sep 30, 2018

Resumed CI: https://ci.nodejs.org/job/node-test-pull-request/17512/ (:heavy_check_mark:)

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 30, 2018
@danbev
Copy link
Contributor

danbev commented Oct 3, 2018

Landed in 1bfd035.

@danbev danbev closed this Oct 3, 2018
danbev pushed a commit that referenced this pull request Oct 3, 2018
There is a race condition between onStreamCloseResponse(), which
removes the wantTrailers listener, and Http2Stream.close(), which
will invalidate the connection. IE, sendTrailers can be called on
a closed connection which would crash with a:
Error [ERR_HTTP2_INVALID_STREAM]: The stream has been destroyed

PR-URL: #23146
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Oct 4, 2018
There is a race condition between onStreamCloseResponse(), which
removes the wantTrailers listener, and Http2Stream.close(), which
will invalidate the connection. IE, sendTrailers can be called on
a closed connection which would crash with a:
Error [ERR_HTTP2_INVALID_STREAM]: The stream has been destroyed

PR-URL: #23146
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@damianobarbati
Copy link

@danbev is there an expected release date for v10.11.1 (or v10.12.0) shipping this fix?

jasnell pushed a commit that referenced this pull request Oct 17, 2018
There is a race condition between onStreamCloseResponse(), which
removes the wantTrailers listener, and Http2Stream.close(), which
will invalidate the connection. IE, sendTrailers can be called on
a closed connection which would crash with a:
Error [ERR_HTTP2_INVALID_STREAM]: The stream has been destroyed

PR-URL: #23146
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@addaleax addaleax mentioned this pull request Oct 20, 2018
2 tasks
@StephenLynx
Copy link

I'm getting this issue sometimes when trying to write headers using http2. Node 10.13 on CentOS 7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants