Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix test-http2-session-timeout.js and move to parallel #24877

Closed
wants to merge 2 commits into from
Closed

Fix test-http2-session-timeout.js and move to parallel #24877

wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 6, 2018

First commit:

test: make http2 timeout test robust

Instead of using magic values for the server timeout in
test-http2-session-timeout, measure the duration of the first request
(which should be longer than subsequent requests) and use that value.

Fixes: https://github.com/nodejs/node/issues/20628

Second commit:

test: move http2 test to parallel

A fix to test-http2-session-timeout makes it sufficiently robust that it
can be moved to the parallel directory.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1856/pipeline

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 6, 2018
@Trott Trott added the http2 Issues or PRs related to the http2 subsystem. label Dec 6, 2018
@Trott
Copy link
Member Author

Trott commented Dec 6, 2018

The test now passes under heavy load in parallel:

$ tools/test.py --repeat 192 -j 96 test/parallel/test-http2-session-timeout.js 
[00:18|% 100|+ 192|-   0]: Done 
$

On master, moving it to parallel makes it trivial to break with even a relatively small load:

$ tools/test.py --repeat 16 -j 16 test/parallel/test-http2-session-timeout.js 
=== release test-http2-session-timeout ===                   
Path: parallel/test-http2-session-timeout
assert.js:128
  throw err;
  ^

AssertionError [ERR_ASSERTION]: Timeout after 40 request(s)
    at Http2Server.mustNotCall (/Users/trott/io.js/test/parallel/test-http2-session-timeout.js:13:10)
    at Http2Server.emit (events.js:189:13)
    at ServerHttp2Session.sessionOnTimeout (internal/http2/core.js:2622:15)
    at Object.onceWrapper (events.js:277:13)
    at ServerHttp2Session.emit (events.js:189:13)
    at ServerHttp2Session._onTimeout (internal/http2/core.js:1307:10)
    at listOnTimeout (timers.js:324:15)
    at processTimers (timers.js:268:5)
Command: out/Release/node /Users/trott/io.js/test/parallel/test-http2-session-timeout.js
=== release test-http2-session-timeout ===                   
Path: parallel/test-http2-session-timeout
assert.js:128
  throw err;
  ^

AssertionError [ERR_ASSERTION]: Timeout after 130 request(s)
    at Http2Server.mustNotCall (/Users/trott/io.js/test/parallel/test-http2-session-timeout.js:13:10)
    at Http2Server.emit (events.js:189:13)
    at ServerHttp2Session.sessionOnTimeout (internal/http2/core.js:2622:15)
    at Object.onceWrapper (events.js:277:13)
    at ServerHttp2Session.emit (events.js:189:13)
    at ServerHttp2Session._onTimeout (internal/http2/core.js:1307:10)
    at listOnTimeout (timers.js:324:15)
    at processTimers (timers.js:268:5)
Command: out/Release/node /Users/trott/io.js/test/parallel/test-http2-session-timeout.js
[00:01|% 100|+  14|-   2]: Done                              
$

@Trott Trott mentioned this pull request Dec 6, 2018
Closed
@Trott
test: make http2 timeout test robust
59bf8fa 
Instead of using magic values for the server timeout in
test-http2-session-timeout, measure the duration of the first request
(which should be longer than subsequent requests) and use that value.

Fixes: nodejs#20628
@Trott
test: move http2 test to parallel
9c93c03 
A fix to test-http2-session-timeout makes it sufficiently robust that it
can be moved to the parallel directory.
@Trott
Copy link
Member Author

Trott commented Dec 6, 2018

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

mcollina
mcollina approved these changes Dec 6, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, brilliant technique!

@Trott
Copy link
Member Author

Trott commented Dec 7, 2018 •
edited
Loading

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19289/ ✔️

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 7, 2018
@Trott
Copy link
Member Author

Trott commented Dec 8, 2018

Landed in 008b904...9df18ad

@Trott Trott closed this Dec 8, 2018
Trott added a commit to Trott/io.js that referenced this pull request Dec 8, 2018
@Trott
test: make http2 timeout test robust
3fe00ef 
Instead of using magic values for the server timeout in
test-http2-session-timeout, measure the duration of the first request
(which should be longer than subsequent requests) and use that value.

Fixes: nodejs#20628

PR-URL: nodejs#24877
Fixes: nodejs#20628
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request Dec 8, 2018
@Trott
test: move http2 test to parallel
9df18ad 
A fix to test-http2-session-timeout makes it sufficiently robust that it
can be moved to the parallel directory.

PR-URL: nodejs#24877
Fixes: nodejs#20628
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 17, 2018
@Trott @BethGriggs
test: make http2 timeout test robust
91ce957 
Instead of using magic values for the server timeout in
test-http2-session-timeout, measure the duration of the first request
(which should be longer than subsequent requests) and use that value.

Fixes: #20628

PR-URL: #24877
Fixes: #20628
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 17, 2018
@Trott @BethGriggs
test: move http2 test to parallel
592bad1 
A fix to test-http2-session-timeout makes it sufficiently robust that it
can be moved to the parallel directory.

PR-URL: #24877
Fixes: #20628
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 18, 2018
Merged
@beevelop beevelop mentioned this pull request Dec 18, 2018
Closed
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@Trott @refack
test: make http2 timeout test robust
5d651a7 
Instead of using magic values for the server timeout in
test-http2-session-timeout, measure the duration of the first request
(which should be longer than subsequent requests) and use that value.

Fixes: nodejs#20628

PR-URL: nodejs#24877
Fixes: nodejs#20628
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@Trott @refack
test: move http2 test to parallel
7151a3e 
A fix to test-http2-session-timeout makes it sufficiently robust that it
can be moved to the parallel directory.

PR-URL: nodejs#24877
Fixes: nodejs#20628
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 12, 2019
@Trott @BethGriggs
test: make http2 timeout test robust
1a1811d 
Instead of using magic values for the server timeout in
test-http2-session-timeout, measure the duration of the first request
(which should be longer than subsequent requests) and use that value.

Fixes: #20628

PR-URL: #24877
Fixes: #20628
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 12, 2019
@Trott @BethGriggs
test: move http2 test to parallel
e20ad2e 
A fix to test-http2-session-timeout makes it sufficiently robust that it
can be moved to the parallel directory.

PR-URL: #24877
Fixes: #20628
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Feb 20, 2019
@Trott @BethGriggs
test: make http2 timeout test robust
5c27278 
Instead of using magic values for the server timeout in
test-http2-session-timeout, measure the duration of the first request
(which should be longer than subsequent requests) and use that value.

Fixes: #20628

PR-URL: #24877
Fixes: #20628
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 20, 2019
@Trott @BethGriggs
test: move http2 test to parallel
1ef8bcb 
A fix to test-http2-session-timeout makes it sufficiently robust that it
can be moved to the parallel directory.

PR-URL: #24877
Fixes: #20628
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@Trott @rvagg
test: make http2 timeout test robust
258f95c 
Instead of using magic values for the server timeout in
test-http2-session-timeout, measure the duration of the first request
(which should be longer than subsequent requests) and use that value.

Fixes: #20628

PR-URL: #24877
Fixes: #20628
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@Trott @rvagg
test: move http2 test to parallel
926b664 
A fix to test-http2-session-timeout makes it sufficiently robust that it
can be moved to the parallel directory.

PR-URL: #24877
Fixes: #20628
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@Trott Trott deleted the fix-20628 branch January 13, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apapirovski apapirovski apapirovski approved these changes

@mcollina mcollina mcollina approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http2 Issues or PRs related to the http2 subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@Trott @nodejs-github-bot @apapirovski @mcollina @lpinca @cjihrig @trivikr @BethGriggs