http2: strictly limit number of concurrent streams #16766

jasnell · 2017-11-05T07:43:20Z

Strictly limit the number of concurrent streams based on the
current setting the MAX_CONCURRENT_STREAMS setting.

Checklist

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

Affected core subsystem(s)

http2

mcollina

LGTM

addaleax · 2017-11-05T14:34:50Z

src/node_http2_core-inl.h

+          NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS);
+  uint32_t maxSize =
+      std::min(static_cast<uint32_t>(streams_.max_size()),
+               maxConcurrentStreams);


If this works it’s going to be implementation-dependent luck, wrapping the size_t to an uint32_t is going to truncate the important bits away on 64-bit platforms. I think upcasting maxConcurrentStreams to size_t is what you’d rather want here, especially since you’re comparing streams_.size() with the result, which generally is a size_t too

Also, common core style is snake_case for local variables

Yeah, good point.

sebdeckers

LGTM, just nit/question.

sebdeckers · 2017-11-06T03:08:25Z

test/parallel/test-http2-too-many-streams.js

+server.listen(0, common.mustCall(() => {
+  const client = http2.connect(`http://localhost:${server.address().port}`);
+
+  client.on('sessionError', console.log);


Are these logs needed? To be explicit maybe wrap them in common.must(Not)Call.

oh, no, they're not. just forgot to remove them

jasnell · 2017-11-12T21:52:43Z

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

jasnell · 2017-11-13T18:57:12Z

Failed on osx due to a flaky condition, updated and running again: https://ci.nodejs.org/job/node-test-pull-request/11398/

jasnell · 2017-11-13T20:40:06Z

Trying again on OSX: https://ci.nodejs.org/job/node-test-pull-request/11402/

jasnell · 2017-11-14T00:59:22Z

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

jasnell · 2017-11-14T01:47:25Z

CI is green now.

@mcollina ... PTAL

mcollina · 2017-11-14T08:48:25Z

test/parallel/test-http2-too-many-streams.js

+      //                is being handled on the server side. This is safe
+      //                to ignore on Windows because of the assert.strictEqual
+      //                check in the on('stream') handler which ensures that
+      //                only one request is being handled at any given time.


I don't think this is safe to ignore. IMHO it is part of the error handling refactoring that we talked about. All client in all OS must get an error in this condition. This test is about the server-side, so it's ok. If we have an issue to track the error handling refactoring, link it.

The comment is a bit incorrect, we're not so much ignoring an error, we're just not expecting one. I'll be investigating further on why later on today, but on windows this second request is serialized out after the first response is received -- which is what should be happening. I actually think it's a bug on the posix side. The test is working exactly as it should. I'm not planning on landing this until I track down the specific issue.

jasnell · 2017-11-22T15:02:06Z

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

jasnell · 2017-11-22T19:22:23Z

TODO: ci failed on OSX also. Will investigate

Strictly limit the number of concurrent streams based on the current setting of the MAX_CONCURRENT_STREAMS setting

jasnell · 2018-01-02T22:08:52Z

updated and rebased

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

jasnell · 2018-01-02T22:55:32Z

CI is good. Getting this landed

Strictly limit the number of concurrent streams based on the current setting of the MAX_CONCURRENT_STREAMS setting PR-URL: #16766 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>

jasnell · 2018-01-02T23:00:07Z

Landed in 653e894

Strictly limit the number of concurrent streams based on the current setting of the MAX_CONCURRENT_STREAMS setting PR-URL: #16766 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>

Strictly limit the number of concurrent streams based on the current setting of the MAX_CONCURRENT_STREAMS setting PR-URL: nodejs#16766 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>

Strictly limit the number of concurrent streams based on the current setting of the MAX_CONCURRENT_STREAMS setting Backport-PR-URL: #18050 PR-URL: #16766 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>

Strictly limit the number of concurrent streams based on the current setting of the MAX_CONCURRENT_STREAMS setting Backport-PR-URL: nodejs#18050 PR-URL: nodejs#16766 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>

Strictly limit the number of concurrent streams based on the current setting of the MAX_CONCURRENT_STREAMS setting Backport-PR-URL: #18050 Backport-PR-URL: #20456 PR-URL: #16766 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>

jasnell requested review from apapirovski, mcollina, sebdeckers and addaleax November 5, 2017 07:43

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 Nov 5, 2017

mcollina approved these changes Nov 5, 2017

View reviewed changes

mscdex added the http2 Issues or PRs related to the http2 subsystem. label Nov 5, 2017

apapirovski approved these changes Nov 5, 2017

View reviewed changes

addaleax reviewed Nov 5, 2017

View reviewed changes

sebdeckers approved these changes Nov 6, 2017

View reviewed changes

jasnell force-pushed the http2-limit-concurrent-streams branch from d3ec486 to f55d8e9 Compare November 12, 2017 21:33

jasnell force-pushed the http2-limit-concurrent-streams branch from f8767f0 to 62e8632 Compare November 14, 2017 00:58

mcollina reviewed Nov 14, 2017

View reviewed changes

jasnell force-pushed the http2-limit-concurrent-streams branch from 62e8632 to 27cebcc Compare November 21, 2017 18:53

nodejs deleted a comment Nov 22, 2017

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

http2: strictly limit number on concurrent streams

22eb93b

Strictly limit the number of concurrent streams based on the current setting of the MAX_CONCURRENT_STREAMS setting

jasnell force-pushed the http2-limit-concurrent-streams branch from 991b61c to 22eb93b Compare January 2, 2018 22:07

jasnell closed this Jan 2, 2018

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added the dont-land-on-v6.x label Jan 19, 2018

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: strictly limit number of concurrent streams #16766

http2: strictly limit number of concurrent streams #16766

jasnell commented Nov 5, 2017

mcollina left a comment

addaleax Nov 5, 2017

jasnell Nov 5, 2017

sebdeckers left a comment

sebdeckers Nov 6, 2017

jasnell Nov 6, 2017

jasnell commented Nov 12, 2017

jasnell commented Nov 13, 2017

jasnell commented Nov 13, 2017

jasnell commented Nov 14, 2017

jasnell commented Nov 14, 2017

mcollina Nov 14, 2017

jasnell Nov 14, 2017

jasnell commented Nov 22, 2017

jasnell commented Nov 22, 2017

jasnell commented Jan 2, 2018

jasnell commented Jan 2, 2018

jasnell commented Jan 2, 2018

http2: strictly limit number of concurrent streams #16766

http2: strictly limit number of concurrent streams #16766

Conversation

jasnell commented Nov 5, 2017

Checklist

Affected core subsystem(s)

mcollina left a comment

Choose a reason for hiding this comment

addaleax Nov 5, 2017

Choose a reason for hiding this comment

jasnell Nov 5, 2017

Choose a reason for hiding this comment

sebdeckers left a comment

Choose a reason for hiding this comment

sebdeckers Nov 6, 2017

Choose a reason for hiding this comment

jasnell Nov 6, 2017

Choose a reason for hiding this comment

jasnell commented Nov 12, 2017

jasnell commented Nov 13, 2017

jasnell commented Nov 13, 2017

jasnell commented Nov 14, 2017

jasnell commented Nov 14, 2017

mcollina Nov 14, 2017

Choose a reason for hiding this comment

jasnell Nov 14, 2017

Choose a reason for hiding this comment

jasnell commented Nov 22, 2017

jasnell commented Nov 22, 2017

jasnell commented Jan 2, 2018

jasnell commented Jan 2, 2018

jasnell commented Jan 2, 2018