FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http2: no stream destroy while its data is on the wire by addaleax · Pull Request #19002 · nodejs/node · GitHub

/ node Public

http2: no stream destroy while its data is on the wire - #19002

Closed
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:fix-18973
ConversationCommits (2)ChecksFiles changed
Closed

http2: no stream destroy while its data is on the wire#19002
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:fix-18973

Conversation

addaleax commented Feb 26, 2018

Copy link
Copy Markdown
Member

This fixes a crash that occurred when a Http2Stream write
is completed after it is already destroyed.

Instead, don’t destroy the stream in that case and wait for
GC to take over.

Fixes: #18973

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

http2

http2: no stream destroy while its data is on the wire
e66021e 
This fixes a crash that occurred when a `Http2Stream` write
is completed after it is already destroyed.

Instead, don’t destroy the stream in that case and wait for
GC to take over.

Fixes: nodejs#18973
nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 26, 2018
addaleax mentioned this pull request Feb 26, 2018
Closed
addaleax added http2 Issues or PRs related to the http2 subsystem. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 26, 2018

BridgeAR commented Mar 1, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 1, 2018
BridgeAR requested review from apapirovski and mcollina March 1, 2018 22:31

addaleax commented Mar 2, 2018

Copy link
Copy Markdown
Member Author

I think that CI was started for a different PR.

CI: https://ci.nodejs.org/job/node-test-commit/16594/

BridgeAR commented Mar 2, 2018

Copy link
Copy Markdown
Member

@addaleax thanks for noticing. It seems like some times the CI window is just refreshed instead of starting a new CI even though everything is entered correct. I guess I did not notice it in that moment.

apapirovski approved these changes Mar 2, 2018
View reviewed changes
Comment thread src/node_http2.cc Outdated
}

bool Http2Session::HasWritesOnSocketForStream(Http2Stream* stream) {
for (const nghttp2_stream_write& wr : outgoing_buffers_)

apapirovski Mar 2, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: can we put brackets around this multi-line block?

addaleax Mar 2, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@apapirovski Sure, done. We’re not that strict with that in the C++ parts usually

apapirovski Mar 2, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I would prefer we were, haha. It's just easier to read :) Thanks!

Comment thread test/parallel/test-http2-write-finishes-after-stream-destroy.js

// Make sure the Http2Stream destructor works, since we don't clean the
// stream up like we would otherwise do.
process.on('exit', global.gc);

apapirovski Mar 2, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm not sure I fully get why the explicit gc is needed in this test. Could you elaborate? Are you just testing for a crash?

addaleax Mar 2, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@apapirovski Yea, I had to make changes to my original patch for this so that this gc() call wouldn’t crash in the destructor.

So yes, it’s not needed, it’s just an extra test

apapirovski Mar 2, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ok, awesome. Just wanted to make sure I wasn't missing anything. Thank you for replying. :)

apapirovski commented Mar 4, 2018

Copy link
Copy Markdown
Contributor

Landed in 584cfc9

apapirovski closed this Mar 4, 2018
apapirovski pushed a commit that referenced this pull request Mar 4, 2018
http2: no stream destroy while its data is on the wire
584cfc9 
This fixes a crash that occurred when a `Http2Stream` write
is completed after it is already destroyed.

Instead, don’t destroy the stream in that case and wait for
GC to take over.

PR-URL: #19002
Fixes: #18973
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

MylesBorins commented Mar 6, 2018

Copy link
Copy Markdown
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

addaleax deleted the fix-18973 branch March 6, 2018 22:04
addaleax added a commit to addaleax/node that referenced this pull request Mar 6, 2018
http2: no stream destroy while its data is on the wire
8827b71 
This fixes a crash that occurred when a `Http2Stream` write
is completed after it is already destroyed.

Instead, don’t destroy the stream in that case and wait for
GC to take over.

PR-URL: nodejs#19002
Fixes: nodejs#18973
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
addaleax mentioned this pull request Mar 6, 2018
Closed
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
http2: no stream destroy while its data is on the wire
3038c9c 
This fixes a crash that occurred when a `Http2Stream` write
is completed after it is already destroyed.

Instead, don’t destroy the stream in that case and wait for
GC to take over.

Backport-PR-URL: #19185
PR-URL: #19002
Fixes: #18973
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
http2: no stream destroy while its data is on the wire
759b210 
This fixes a crash that occurred when a `Http2Stream` write
is completed after it is already destroyed.

Instead, don’t destroy the stream in that case and wait for
GC to take over.

Backport-PR-URL: #19185
PR-URL: #19002
Fixes: #18973
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
http2: no stream destroy while its data is on the wire
4e9ac10 
This fixes a crash that occurred when a `Http2Stream` write
is completed after it is already destroyed.

Instead, don’t destroy the stream in that case and wait for
GC to take over.

Backport-PR-URL: #19185
PR-URL: #19002
Fixes: #18973
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins mentioned this pull request Mar 6, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 7, 2018
http2: no stream destroy while its data is on the wire
50d1233 
This fixes a crash that occurred when a `Http2Stream` write
is completed after it is already destroyed.

Instead, don’t destroy the stream in that case and wait for
GC to take over.

Backport-PR-URL: #19185
PR-URL: #19002
Fixes: #18973
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins added a commit that referenced this pull request Mar 7, 2018
2018-03-07, Version 9.8.0 (Current)
27ba6e2 
Notable Changes:

* crypto:
  - add cert.fingerprint256 as SHA256 fingerprint (Hannes Magnusson)
    #17690
* http2:
  - Fixed issues with aborted connections in the HTTP/2 implementation
    (Anna Henningsen)
    #18987
    #19002
* loader:
  - --inspect-brk now works properly for esmodules (Gus Caplan)
    #18949
* src:
  - make process.dlopen() load well-known symbol (Ben Noordhuis)
    #18934
* trace_events:
  - add file pattern cli option (Andreas Madsen)
    #18480
* Added new collaborators:
  - Chen Gang (MoonBall) https://github.com/MoonBall

PR-URL: #19181
MylesBorins added a commit that referenced this pull request Mar 8, 2018
2018-03-07, Version 9.8.0 (Current)
282e65c 
Notable Changes:

* crypto:
  - add cert.fingerprint256 as SHA256 fingerprint (Hannes Magnusson)
    #17690
* http2:
  - Fixed issues with aborted connections in the HTTP/2 implementation
    (Anna Henningsen)
    #18987
    #19002
* loader:
  - --inspect-brk now works properly for esmodules (Gus Caplan)
    #18949
* src:
  - make process.dlopen() load well-known symbol (Ben Noordhuis)
    #18934
* trace_events:
  - add file pattern cli option (Andreas Madsen)
    #18480
* Added new collaborators:
  - Chen Gang (MoonBall) https://github.com/MoonBall

PR-URL: #19181
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
http2: no stream destroy while its data is on the wire
4188136 
This fixes a crash that occurred when a `Http2Stream` write
is completed after it is already destroyed.

Instead, don’t destroy the stream in that case and wait for
GC to take over.

Backport-PR-URL: nodejs#19185
PR-URL: nodejs#19002
Fixes: nodejs#18973
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
http2: no stream destroy while its data is on the wire
f165e9f 
This fixes a crash that occurred when a `Http2Stream` write
is completed after it is already destroyed.

Instead, don’t destroy the stream in that case and wait for
GC to take over.

Backport-PR-URL: #19185
Backport-PR-URL: #20456
PR-URL: #19002
Fixes: #18973
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
http2: no stream destroy while its data is on the wire
cbeec24 
This fixes a crash that occurred when a `Http2Stream` write
is completed after it is already destroyed.

Instead, don’t destroy the stream in that case and wait for
GC to take over.

PR-URL: nodejs#19002
Fixes: nodejs#18973
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
2018-03-07, Version 9.8.0 (Current)
1874c27 
Notable Changes:

* crypto:
  - add cert.fingerprint256 as SHA256 fingerprint (Hannes Magnusson)
    nodejs#17690
* http2:
  - Fixed issues with aborted connections in the HTTP/2 implementation
    (Anna Henningsen)
    nodejs#18987
    nodejs#19002
* loader:
  - --inspect-brk now works properly for esmodules (Gus Caplan)
    nodejs#18949
* src:
  - make process.dlopen() load well-known symbol (Ben Noordhuis)
    nodejs#18934
* trace_events:
  - add file pattern cli option (Andreas Madsen)
    nodejs#18480
* Added new collaborators:
  - Chen Gang (MoonBall) https://github.com/MoonBall

PR-URL: nodejs#19181
MylesBorins pushed a commit that referenced this pull request May 15, 2018
http2: no stream destroy while its data is on the wire
7321dd3 
This fixes a crash that occurred when a `Http2Stream` write
is completed after it is already destroyed.

Instead, don’t destroy the stream in that case and wait for
GC to take over.

Backport-PR-URL: #19185
Backport-PR-URL: #20456
PR-URL: #19002
Fixes: #18973
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
http2: no stream destroy while its data is on the wire
c976cb5 
This fixes a crash that occurred when a `Http2Stream` write
is completed after it is already destroyed.

Instead, don’t destroy the stream in that case and wait for
GC to take over.

Backport-PR-URL: #19185
Backport-PR-URL: #20456
PR-URL: #19002
Fixes: #18973
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
hermanbanken mentioned this pull request Nov 2, 2018
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
mcollina Awaiting requested review from mcollina
+1 more reviewer
apapirovski apapirovski approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Segmentation fault in http2 module when cancelling download in Chrome

6 participants

Back | FazBrowse Home | New Git URL