Conversation
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 |
| stream.read(0); | ||
| } | ||
| if (!stream[kState].didRead && !stream._readableState.resumeScheduled) | ||
| stream.resume(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can you please add a comment about this?
There was a problem hiding this comment.
Will do later today.
| state.didRead = true; | ||
| this[kStream].on('data', onStreamData); | ||
| } else { | ||
| process.nextTick(resumeStream, this[kStream]); |
There was a problem hiding this comment.
I don't think the nextTick is needed here, as resume() happens in a nextTick anyway.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You could probably have a look at the git blame for the line to find the corresponding PR.
There was a problem hiding this comment.
See #15503 — I don't fully recall where exactly we have an async call to pause but we do somewhere.
There was a problem hiding this comment.
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:
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. 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. 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>
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), orvcbuild test(Windows) passes