http2: strictly limit number of concurrent streams#16766
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added c++ 
src/node_http2_core-inl.h Outdated
There was a problem hiding this comment.
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
sebdeckers left a comment
sebdeckers left a comment There was a problem hiding this comment.
LGTM, just nit/question.
There was a problem hiding this comment.
Are these logs needed? To be explicit maybe wrap them in common.must(Not)Call.
There was a problem hiding this comment.
oh, no, they're not. just forgot to remove them
jasnellforce-pushed the http2-limit-concurrent-streams branch from d3ec486 to f55d8e9Compare
jasnell commented Nov 12, 2017
jasnell commented Nov 13, 2017
Failed on osx due to a flaky condition, updated and running again: https://ci.nodejs.org/job/node-test-pull-request/11398/ |
jasnell commented Nov 13, 2017
Trying again on OSX: https://ci.nodejs.org/job/node-test-pull-request/11402/ |
jasnellforce-pushed the http2-limit-concurrent-streams branch from f8767f0 to 62e8632Comparejasnell commented Nov 14, 2017
jasnell commented Nov 14, 2017
CI is green now. @mcollina ... PTAL |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
jasnellforce-pushed the http2-limit-concurrent-streams branch from 62e8632 to 27cebccComparejasnell commented Nov 22, 2017
jasnell commented Nov 22, 2017
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
jasnellforce-pushed the http2-limit-concurrent-streams branch from 991b61c to 22eb93bComparejasnell commented Jan 2, 2018
updated and rebased |
jasnell commented Jan 2, 2018
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>
jasnell commented Jan 2, 2018
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Sebastiaan Deckers <[email protected]>
8 participants
Strictly limit the number of concurrent streams based on the
current setting the MAX_CONCURRENT_STREAMS setting.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
http2