http2: fix end without read#20621

apapirovski · 2018-05-09T10:19:54Z

Adjust http2 behaviour to allow ending a stream even after some
data comes in (when the user has no intention of reading that
data). Also correctly end a stream when trailers are present.

Fixes: #20060

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Adjust http2 behaviour to allow ending a stream even after some data comes in (when the user has no intention of reading that data). Also correctly end a stream when trailers are present.

apapirovski · 2018-05-09T10:21:22Z

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

/cc @nodejs/http2

jasnell

LGTM, would like @mcollina to review also :-)

apapirovski · 2018-05-16T06:35:49Z

@mcollina since @jasnell requested your review, could you have a look? Thanks!

mcollina

LGTM

mcollina · 2018-05-16T15:50:47Z

lib/internal/http2/core.js

-stream.read(0);
-}
+if(!stream[kState].didRead&&!stream._readableState.resumeScheduled)
+stream.resume();


this is slightly different than read(0). Can you please explain why this is needed?
I would prefer if we didn't use _readableState here.

This is equivalent to what we do in http and is needed because in cases where the user doesn't intend to consume the data (indicated by never having read and not having a pending resume call), we still want to be able to properly destroy the stream.

Can you please add a comment about this?

Will do later today.

mcollina · 2018-05-16T15:51:25Z

lib/internal/http2/compat.js

+state.didRead=true;
+this[kStream].on('data',onStreamData);
+}else{
+process.nextTick(resumeStream,this[kStream]);


I don't think the nextTick is needed here, as resume() happens in a nextTick anyway.

That part is carried over from before (didn't change) and used to definitely be necessary. We have a test for it even which breaks otherwise: parallel/test-http2-compat-serverrequest-pipe.

You could probably have a look at the git blame for the line to find the corresponding PR.

See #15503 — I don't fully recall where exactly we have an async call to pause but we do somewhere.

It's probably something we would have to look into. It's a pretty old one: #15702.

apapirovski · 2018-05-17T09:16:23Z

Updated with the requested comment. Will be landing this later today.

apapirovski · 2018-05-17T14:00:06Z

Landed in 8d38288. Thanks everyone!

Adjust http2 behaviour to allow ending a stream even after some data comes in (when the user has no intention of reading that data). Also correctly end a stream when trailers are present. PR-URL: #20621Fixes: #20060 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

addaleax · 2018-05-18T23:32:18Z

@apapirovski I think there’s a pretty high chance this is the cause of CI failures like these, given that this occurred somewhat frequently only today and this PR touched the test: https://ci.nodejs.org/job/node-test-commit-linuxone/1405/nodes=rhel72-s390x/testReport/junit/(root)/test/parallel_test_http2_client_upload_reject/

BridgeAR · 2018-05-19T00:01:33Z

@addaleax I do not think that this is the cause of these issues. We have these failures since a couple of days and this PR just landed very recently.

BridgeAR · 2018-05-19T00:02:46Z

See #20750

addaleax · 2018-05-19T00:23:34Z

@BridgeAR Locally, for that specific test (which is not mentioned in the issue), I get:

a 25/1000 failure rate on master
a 19/1000 failure rate for the commit in which this PR landed, and
a 0/1000 failure rate for its parent.

That makes me somewhat confident that this PR is in fact the cause of what we’ve been seeing today?

BridgeAR · 2018-05-19T00:44:24Z

I just had another look into it and you are right! I opened a revert #20832

Adjust http2 behaviour to allow ending a stream even after some data comes in (when the user has no intention of reading that data). Also correctly end a stream when trailers are present. PR-URL: #20621Fixes: #20060 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

Adjust http2 behaviour to allow ending a stream even after some data comes in (when the user has no intention of reading that data). Also correctly end a stream when trailers are present. PR-URL: nodejs#20621Fixes: nodejs#20060 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

Adjust http2 behaviour to allow ending a stream even after some data comes in (when the user has no intention of reading that data). Also correctly end a stream when trailers are present. Backport-PR-URL: #22850 PR-URL: #20621Fixes: #20060 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

http2: fix end without read
c76cf39
Adjust http2 behaviour to allow ending a stream even after some data comes in (when the user has no intention of reading that data). Also correctly end a stream when trailers are present.

apapirovski added the http2 Issues or PRs related to the http2 subsystem. label May 9, 2018

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 9, 2018

apapirovski mentioned this pull request May 9, 2018
difference between http2 Compatibility API and http #20060
Closed

jasnell approved these changes May 9, 2018
View reviewed changes

jasnell requested a review from mcollina May 9, 2018 17:23

mcollina approved these changes May 16, 2018
View reviewed changes

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

trivikr approved these changes May 16, 2018
View reviewed changes

mcollina reviewed May 16, 2018
View reviewed changes

trivikr removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 17, 2018

fixup: add a comment
504055f

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

apapirovski closed this May 17, 2018

apapirovski deleted the fix-http2-end-without-read branch May 17, 2018 14:00

BridgeAR mentioned this pull request May 19, 2018
Revert "http2: fix end without read" #20832
Closed
4 tasks

addaleax mentioned this pull request May 22, 2018
v10.2.0 proposal #20724
Merged

BethGriggs mentioned this pull request Oct 30, 2018
v8.13.0 proposal #23974
Merged

beevelop mentioned this pull request Nov 20, 2018
2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs beevelop/docker-android-nodejs#250
Closed

Uh oh!

http2: fix end without read#20621

http2: fix end without read #20621

Uh oh!

Conversation

apapirovski commented May 9, 2018

Checklist

Uh oh!

apapirovski commented May 9, 2018

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

apapirovski commented May 16, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollinaMay 16, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski commented May 17, 2018

Uh oh!

apapirovski commented May 17, 2018

Uh oh!

addaleax commented May 18, 2018

Uh oh!

BridgeAR commented May 19, 2018

Uh oh!

BridgeAR commented May 19, 2018

Uh oh!

addaleax commented May 19, 2018

Uh oh!

BridgeAR commented May 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mcollinaMay 16, 2018•
edited
Loading