http2: fix end without read by apapirovski · Pull Request #20621 · nodejs/node · GitHub

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.

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

/cc @nodejs/http2

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

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

LGTM

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.

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.

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

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: #20621 Fixes: #20060 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

@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/

@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.

See #20750

@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?

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: #20621 Fixes: #20060 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

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#20621 Fixes: nodejs#20060 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

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: #20621 Fixes: #20060 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

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 outstanding review comments, 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 outstanding review comments, 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 outstanding review comments, 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

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

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

http2: fix end without read by apapirovski · Pull Request #20621 · nodejs/node · GitHub

Uh oh!

Uh oh!

http2: fix end without read - #20621

http2: fix end without read#20621apapirovski wants to merge 2 commits intonodejs:masternodejs/node:masterfrom apapirovski:fix-http2-end-without-readapapirovski/node:fix-http2-end-without-readCopy head branch name to clipboard

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!

mcollina May 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

http2: fix end without read#20621
apapirovski wants to merge 2 commits into
nodejs:masterfrom
apapirovski:fix-http2-end-without-read

mcollina May 16, 2018 •
edited
Loading