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: #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>

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: #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 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: fix end without read #20621

http2: fix end without read #20621

apapirovski commented May 9, 2018

apapirovski commented May 9, 2018

jasnell left a comment

apapirovski commented May 16, 2018

mcollina left a comment

mcollina May 16, 2018

apapirovski May 16, 2018

mcollina May 16, 2018

apapirovski May 16, 2018

mcollina May 16, 2018

apapirovski May 16, 2018

apapirovski May 16, 2018

apapirovski May 16, 2018

mcollina May 16, 2018 •

edited

Loading

apapirovski commented May 17, 2018

apapirovski commented May 17, 2018

addaleax commented May 18, 2018

BridgeAR commented May 19, 2018

BridgeAR commented May 19, 2018

addaleax commented May 19, 2018

BridgeAR commented May 19, 2018

http2: fix end without read #20621

http2: fix end without read #20621

Conversation

apapirovski commented May 9, 2018

Checklist

apapirovski commented May 9, 2018

jasnell left a comment

Choose a reason for hiding this comment

apapirovski commented May 16, 2018

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina May 16, 2018 • edited Loading

Choose a reason for hiding this comment

apapirovski commented May 17, 2018

apapirovski commented May 17, 2018

addaleax commented May 18, 2018

BridgeAR commented May 19, 2018

BridgeAR commented May 19, 2018

addaleax commented May 19, 2018

BridgeAR commented May 19, 2018

mcollina May 16, 2018 •

edited

Loading