http2: strictly limit number of concurrent streams by jasnell · Pull Request #16766 · nodejs/node

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

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

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

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 addaleax, apapirovski, mcollina and sebdeckers 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 f55d8e9Compare November 12, 2017 21:33

jasnell force-pushed the http2-limit-concurrent-streams branch from f8767f0 to 62e8632Compare 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 27cebccCompare November 21, 2017 18:53

nodejs deleted a comment Nov 22, 2017

MylesBorins force-pushed the master branch from b7405ab to 7f086ddCompare 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 22eb93bCompare 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

Uh oh!

Uh oh!

Conversation

jasnell commented Nov 5, 2017

Checklist

Affected core subsystem(s)

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

addaleaxNov 5, 2017

Choose a reason for hiding this comment

Uh oh!

jasnellNov 5, 2017

Choose a reason for hiding this comment

Uh oh!

sebdeckers left a comment

Choose a reason for hiding this comment

Uh oh!

sebdeckersNov 6, 2017

Choose a reason for hiding this comment

Uh oh!

jasnellNov 6, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Nov 12, 2017

Uh oh!

jasnell commented Nov 13, 2017

Uh oh!

jasnell commented Nov 13, 2017

Uh oh!

jasnell commented Nov 14, 2017

Uh oh!

jasnell commented Nov 14, 2017

Uh oh!

mcollinaNov 14, 2017

Choose a reason for hiding this comment

Uh oh!

jasnellNov 14, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Nov 22, 2017

Uh oh!

jasnell commented Nov 22, 2017

Uh oh!

jasnell commented Jan 2, 2018

Uh oh!

jasnell commented Jan 2, 2018

Uh oh!

jasnell commented Jan 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants