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

addaleax · 2018-02-26T14:46:22Z

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.

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

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

BridgeAR · 2018-03-01T22:31:25Z

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

addaleax · 2018-03-02T10:53:54Z

I think that CI was started for a different PR.

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

BridgeAR · 2018-03-02T12:30:54Z

@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 · 2018-03-02T12:32:31Z

src/node_http2.cc

 }

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


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

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

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

apapirovski · 2018-03-02T12:34:43Z

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);


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?

@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

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

apapirovski · 2018-03-04T12:29:00Z

Landed in 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 · 2018-03-06T21:38:45Z

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.

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>

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>

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

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>

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>

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>

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

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>

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

Segmentation fault in http2 module when cancelling download in Chrome #18973

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

jasnell approved these changes Feb 26, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 1, 2018

BridgeAR requested review from apapirovski and mcollina March 1, 2018 22:31

apapirovski approved these changes Mar 2, 2018

View reviewed changes

[squash] add requested block

4140671

apapirovski closed this Mar 4, 2018

MylesBorins added the backport-requested-v9.x label Mar 6, 2018

addaleax deleted the fix-18973 branch March 6, 2018 22:04

addaleax mentioned this pull request Mar 6, 2018

[v9.x] backport 18656 + 19002 #19185

Closed

addaleax added backported-to-v9.x and removed backport-requested-v9.x labels Mar 6, 2018

MylesBorins mentioned this pull request Mar 6, 2018

v9.8.0 proposal #19181

Merged

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

hermanbanken mentioned this pull request Nov 2, 2018

Node crashes when writing data on HTTP2 stream that meanwhile terminates #24037

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

addaleax commented Feb 26, 2018

Uh oh!

BridgeAR commented Mar 1, 2018 •

edited

Loading

Uh oh!

addaleax commented Mar 2, 2018

Uh oh!

BridgeAR commented Mar 2, 2018

Uh oh!

apapirovski Mar 2, 2018

Uh oh!

addaleax Mar 2, 2018

Uh oh!

apapirovski Mar 2, 2018

Uh oh!

apapirovski Mar 2, 2018

Uh oh!

addaleax Mar 2, 2018

Uh oh!

apapirovski Mar 2, 2018

Uh oh!

apapirovski commented Mar 4, 2018

Uh oh!

MylesBorins commented Mar 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

addaleax commented Feb 26, 2018

Checklist

Affected core subsystem(s)

Uh oh!

BridgeAR commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Mar 2, 2018

Uh oh!

BridgeAR commented Mar 2, 2018

Uh oh!

apapirovski Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovski Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovski Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovski Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Mar 4, 2018

Uh oh!

MylesBorins commented Mar 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

BridgeAR commented Mar 1, 2018 •

edited

Loading