http2: fix responses to long payload reqs #20084

apapirovski · 2018-04-16T20:07:28Z

When a request with a long payload is received, http2 does not allow a response that does not process all the incoming payload if the payload exceeds the initial window size. Add a conditional Http2Stream#close call that runs only if its a server-side stream, the user hasn't attempted to read the stream & there is no resume call scheduled.

With all that said, I hope someone has some good ideas that will make this less of a monster...

Also, FWIW this should make writes slightly less brittle because right now it's possible to trigger a follow-up write from the Done callback which would then trigger the assert in SendPendingData. In particular this tends to happen when using either destroy or close on a stream.

Fixes: #20060

Checklist

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

apapirovski · 2018-04-16T22:51:17Z

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

apapirovski · 2018-04-16T23:39:55Z

Have to update this to support the new trailers implementation.

apapirovski · 2018-04-17T00:17:20Z

There. That should get it compatible with the trailers stuff. Please review again if you could @addaleax & @jasnell. (Just the last commit is new.)

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

apapirovski · 2018-04-17T08:43:22Z

Ok, one related failure in parallel/test-http2-serve-file.js. Investigating.

Edit: I think this might be not directly related to this change but rather these changes are exposing another existing bug? Debugging. Hopefully have a fix soon.

apapirovski · 2018-04-17T13:55:32Z

Ok, so there was indeed an unrelated issue around not flushing pending data when sending RST_STREAM. This commit takes care of that: 1f29ad5

This is tied into how nghttp2 handles RST_STREAM frames: nghttp2/nghttp2#692 (with higher priority than others).

Definitely open to suggestions on how to do it better!

apapirovski · 2018-04-17T13:58:18Z

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

apapirovski · 2018-04-17T13:58:47Z

test/parallel/test-http2-perf_hooks.js

@@ -26,7 +26,7 @@ const obs = new PerformanceObserver(common.mustCall((items) => {
      switch (entry.type) {
        case 'server':
          assert.strictEqual(entry.streamCount, 1);
-          assert.strictEqual(entry.framesReceived, 5);


This is already flaky on master but exacerbated by some of the changes here. I think it might be timing sensitive? Minimum seems to be 3.

+1 for the fix, can you add a comment?

apapirovski · 2018-04-17T14:53:45Z

Failure in https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1604_sharedlibs_debug_x64/3797/tapResults/ seems potentially related but no clue how I'm causing SIGABRT...

Let's try something... CI: https://ci.nodejs.org/job/node-test-pull-request/14352/

Stress test CI: https://ci.nodejs.org/job/node-stress-single-test/1819/nodes=ubuntu1604-64/console

apapirovski · 2018-04-17T17:34:52Z

This seems clean now of any hidden bugs. ping @nodejs/http2 @addaleax for another review. In particular these two commits: 1f29ad5 and 1ff5bc9. Thanks in advance!

mcollina

LGTM

ryzokuken · 2018-04-18T18:32:25Z

@BridgeAR still awaiting your final review on this before landing.

BridgeAR · 2018-04-18T18:37:50Z

@ryzokuken I guess you meant @addaleax?

ryzokuken · 2018-04-18T20:11:42Z

@BridgeAR wait, sorry. Wrong thread. I need more rest, I think.

ryzokuken · 2018-04-18T20:14:52Z

P.S. It was #19898

jasnell · 2018-04-19T16:39:18Z

If this is going to make it in to 10.0.0 it really needs to get landed today.

apapirovski · 2018-04-19T16:48:41Z

@jasnell it's ready to go on my end (I think?) but I could really use one more review for the C++ stuff from you or @addaleax.

Trott · 2018-04-20T17:34:50Z

plinux CI rerun: https://ci.nodejs.org/job/node-test-commit-plinux/17081/

apapirovski · 2018-04-28T06:17:54Z

Last CI run before landing: https://ci.nodejs.org/job/node-test-pull-request/14564/

apapirovski · 2018-04-28T16:17:53Z

Landed in b55a11d

When a request with a long payload is received, http2 does not allow a response that does not process all the incoming payload. Add a conditional Http2Stream.close call that runs only if the user hasn't attempted to read the stream. PR-URL: #20084 Fixes: #20060 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

When a request with a long payload is received, http2 does not allow a response that does not process all the incoming payload. Add a conditional Http2Stream.close call that runs only if the user hasn't attempted to read the stream. PR-URL: nodejs#20084 Fixes: nodejs#20060 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

kjin · 2018-09-11T01:29:44Z

@apapirovski I've been trying to do a backport of http2 commits to 8.x, but this commit has been giving me some trouble, as its passing tests is reliant on a PR (#18438) that is marked as semver-major. A number of following PRs depend on this one, so this seems to be a big factor in what can be backported correctly. What are your thoughts on this? (cc/ @jasnell, @mcollina, @addaleax)

apapirovski · 2018-09-11T01:55:13Z

@kjin I will have a look at it and see if I can help out. Do you have a branch you're working against that I could pull down to do my work on?

kjin · 2018-09-11T02:01:17Z

@apapirovski Thanks! The current branch is here and fairly up-to-date against v8.x-staging. If I were to add the commit associated with this PR it would go right on top.

kjin · 2018-09-19T22:17:13Z

Hi @apapirovski -- after going through the rest of the commits, it turns out that backporting the commits in #20772 seems to fix the failing tests from this PR. Given that you are the author of both PRs, does this seem logical to you, or do you know if any change in #20772 would mask the issues arising here?

When a request with a long payload is received, http2 does not allow a response that does not process all the incoming payload. Add a conditional Http2Stream.close call that runs only if the user hasn't attempted to read the stream. PR-URL: nodejs#20084 Fixes: nodejs#20060 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

When a request with a long payload is received, http2 does not allow a response that does not process all the incoming payload. Add a conditional Http2Stream.close call that runs only if the user hasn't attempted to read the stream. Backport-PR-URL: #22850 PR-URL: #20084 Fixes: #20060 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

apapirovski added the addons Issues and PRs related to native addons. label Apr 16, 2018

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 16, 2018

apapirovski requested a review from addaleax April 16, 2018 20:07

apapirovski added http2 Issues or PRs related to the http2 subsystem. and removed addons Issues and PRs related to native addons. labels Apr 16, 2018

apapirovski force-pushed the fix-http2-bug-20060 branch from 9897506 to a6c958f Compare April 16, 2018 20:11

addaleax approved these changes Apr 16, 2018

View reviewed changes

jasnell approved these changes Apr 16, 2018

View reviewed changes

apapirovski force-pushed the fix-http2-bug-20060 branch from 56364b9 to edb4d4c Compare April 17, 2018 00:16

aks- approved these changes Apr 17, 2018

View reviewed changes

addaleax approved these changes Apr 17, 2018

View reviewed changes

apapirovski commented Apr 17, 2018

View reviewed changes

mcollina approved these changes Apr 17, 2018

View reviewed changes

apapirovski added this to the 10.0.0 milestone Apr 18, 2018

apapirovski added 2 commits April 27, 2018 18:44

fixup: account for destroy before connect

08fa98c

fixup: make test less brittle

51adabe

addaleax mentioned this pull request Apr 27, 2018

Install h2load on benchmarking machine nodejs/build#1147

Closed

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

apapirovski force-pushed the fix-http2-bug-20060 branch from 2a9f101 to 2228f59 Compare April 28, 2018 06:17

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

fixup: clear storage early

21f0e82

apapirovski force-pushed the fix-http2-bug-20060 branch from 2228f59 to 21f0e82 Compare April 28, 2018 14:56

apapirovski closed this Apr 28, 2018

apapirovski deleted the fix-http2-bug-20060 branch April 28, 2018 16:17

MylesBorins mentioned this pull request May 8, 2018

v10.1.0 proposal #20606

Merged

kjin mentioned this pull request Oct 3, 2018

[8.x] Backport http2 changes from 10.x #22850

Closed

4 tasks

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 responses to long payload reqs #20084

http2: fix responses to long payload reqs #20084

apapirovski commented Apr 16, 2018

apapirovski commented Apr 16, 2018

apapirovski commented Apr 16, 2018

apapirovski commented Apr 17, 2018

apapirovski commented Apr 17, 2018 •

edited

Loading

apapirovski commented Apr 17, 2018

apapirovski commented Apr 17, 2018

apapirovski Apr 17, 2018

mcollina Apr 17, 2018

apapirovski commented Apr 17, 2018 •

edited

Loading

apapirovski commented Apr 17, 2018 •

edited

Loading

mcollina left a comment

ryzokuken commented Apr 18, 2018

BridgeAR commented Apr 18, 2018

ryzokuken commented Apr 18, 2018

ryzokuken commented Apr 18, 2018

jasnell commented Apr 19, 2018

apapirovski commented Apr 19, 2018

Trott commented Apr 20, 2018

apapirovski commented Apr 28, 2018 •

edited

Loading

apapirovski commented Apr 28, 2018

kjin commented Sep 11, 2018

apapirovski commented Sep 11, 2018

kjin commented Sep 11, 2018

kjin commented Sep 19, 2018

http2: fix responses to long payload reqs #20084

http2: fix responses to long payload reqs #20084

Conversation

apapirovski commented Apr 16, 2018

Checklist

apapirovski commented Apr 16, 2018

apapirovski commented Apr 16, 2018

apapirovski commented Apr 17, 2018

apapirovski commented Apr 17, 2018 • edited Loading

apapirovski commented Apr 17, 2018

apapirovski commented Apr 17, 2018

apapirovski Apr 17, 2018

Choose a reason for hiding this comment

mcollina Apr 17, 2018

Choose a reason for hiding this comment

apapirovski commented Apr 17, 2018 • edited Loading

apapirovski commented Apr 17, 2018 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

ryzokuken commented Apr 18, 2018

BridgeAR commented Apr 18, 2018

ryzokuken commented Apr 18, 2018

ryzokuken commented Apr 18, 2018

jasnell commented Apr 19, 2018

apapirovski commented Apr 19, 2018

Trott commented Apr 20, 2018

apapirovski commented Apr 28, 2018 • edited Loading

apapirovski commented Apr 28, 2018

kjin commented Sep 11, 2018

apapirovski commented Sep 11, 2018

kjin commented Sep 11, 2018

kjin commented Sep 19, 2018

apapirovski commented Apr 17, 2018 •

edited

Loading

apapirovski commented Apr 17, 2018 •

edited

Loading

apapirovski commented Apr 17, 2018 •

edited

Loading

apapirovski commented Apr 28, 2018 •

edited

Loading