GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — http2: no stream destroy while its data is on the wire by addaleax · Pull Request #19002 · nodejs/node

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          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 involving general changes in the lib/ or src/ directories. 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 and PRs related to the http2 subsystem. and removed lib / src Issues and PRs involving general changes in the lib/ or src/ directories. labels

Feb 26, 2018

jasnell approved these changes

Feb 26, 2018

View reviewed changes

BridgeAR commented Mar 1, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

BridgeAR added the author ready PRs with CI started, the required approvals, and no outstanding review comments. 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/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

addaleax Mar 2, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

apapirovski Mar 2, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

addaleax Mar 2, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

apapirovski Mar 2, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          [squash] add requested block

apapirovski commented Mar 4, 2018

Copy link

Copy Markdown

Contributor

Landed in 584cfc9

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>

MylesBorins added the backport-requested-v9.x label

Mar 6, 2018

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>

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 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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>

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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>

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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>

MylesBorins mentioned this pull request

Mar 6, 2018

v9.8.0 proposal #19181

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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>

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

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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>

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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>

MylesBorins mentioned this pull request

May 2, 2018

v8.11.2 proposal #20478

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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>

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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>

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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>

hermanbanken mentioned this pull request

Nov 2, 2018

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

Closed

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

c++

Issues and PRs that require attention from people who are familiar with C++.

http2

Issues and 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

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

BridgeAR commented Mar 1, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.