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

test: fix flaky test-http-set-timeout-server #11790

Merged
merged 1 commit into from
Mar 14, 2017
Merged

test: fix flaky test-http-set-timeout-server #11790

merged 1 commit into from
Mar 14, 2017

Conversation

santigimeno
Copy link
Member

It can happen that the connection and server is closed before the second
reponse has been processed by server. In this case, the
res.setTimeout() callback will never be called causing the test to
fail. Fix this by only closing the connection and server when the 2nd
has been received.

Fixes: #11768

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 10, 2017
jasnell
jasnell reviewed Mar 10, 2017
View reviewed changes
test/parallel/test-http-set-timeout-server.js
@@ -117,20 +117,25 @@ test(function serverRequestNotTimeoutAfterEnd(cb) {

test(function serverResponseTimeoutWithPipeline(cb) {
let caughtTimeout = '';
let secReceived = false;
process.on('exit', function() {
assert.strictEqual(caughtTimeout, '/2');
});
const server = http.createServer(function(req, res) {
Copy link
Member

Choose a reason for hiding this comment

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

may be worthwhile adding in common.mustCall() for the various callbacks in this test.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought of that but it's tricky because the number of times the callbacks are run depends on when the timer fires. I can wrap the http.createServer() callback though

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if I can wrap the http.createServer() callback either as I'm not sure it's going to be called always 3 times.

Copy link
Member

Choose a reason for hiding this comment

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

hmm.. ok.

@hiroppy hiroppy added the http Issues or PRs related to the http subsystem. label Mar 10, 2017
Trott
Trott approved these changes Mar 10, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Nice! LGTM if CI is ✅

@gibfahn
Copy link
Member

gibfahn commented Mar 12, 2017 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-commit/8407/

Stress test on FreeBSD Wrong PR
Stress test on FreeBSD master for comparison

Any other platforms that we should stress test?

EDIT: It seems like the test is failing on the PR branch at a similar frequency to master when run in parallel.

@santigimeno
Copy link
Member Author

@gibfahn the first stress test does not seem to use the code in this PR. I've started a new one: https://ci.nodejs.org/job/node-stress-single-test/1132/

@gibfahn
Copy link
Member

gibfahn commented Mar 13, 2017

@santigimeno sorry about that, had to rerun several times, must have got the PR number confused at some point.

@santigimeno
Copy link
Member Author

Stress test on master failed while stress test with this PR passed.

@santigimeno
test: fix flaky test-http-set-timeout-server
3be1db8 
It can happen that the connection and server is closed before the second
reponse has been processed by server. In this case, the
`res.setTimeout()` callback will never be called causing the test to
fail. Fix this by only closing the connection and server when the 2nd
has been received.

PR-URL: nodejs#11790
Fixes: nodejs#11768
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@santigimeno santigimeno force-pushed the fix_flaky_http-set-timeout-server branch from 8d0434f to 3be1db8 Compare March 14, 2017 08:57
@santigimeno santigimeno merged commit 3be1db8 into nodejs:master Mar 14, 2017
@santigimeno
Copy link
Member Author

Landed in 3be1db8. Thanks.

italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 14, 2017
@santigimeno @italoacasas
test: fix flaky test-http-set-timeout-server
734ddbe 
It can happen that the connection and server is closed before the second
reponse has been processed by server. In this case, the
`res.setTimeout()` callback will never be called causing the test to
fail. Fix this by only closing the connection and server when the 2nd
has been received.

PR-URL: nodejs#11790
Fixes: nodejs#11768
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@italoacasas italoacasas mentioned this pull request Mar 14, 2017
Merged
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
@santigimeno @jungx098
test: fix flaky test-http-set-timeout-server
470a17c 
It can happen that the connection and server is closed before the second
reponse has been processed by server. In this case, the
`res.setTimeout()` callback will never be called causing the test to
fail. Fix this by only closing the connection and server when the 2nd
has been received.

PR-URL: nodejs#11790
Fixes: nodejs#11768
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
@santigimeno @MylesBorins
test: fix flaky test-http-set-timeout-server
9c9f232 
It can happen that the connection and server is closed before the second
reponse has been processed by server. In this case, the
`res.setTimeout()` callback will never be called causing the test to
fail. Fix this by only closing the connection and server when the 2nd
has been received.

PR-URL: #11790
Fixes: #11768
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@santigimeno @MylesBorins
test: fix flaky test-http-set-timeout-server
cd98f5d 
It can happen that the connection and server is closed before the second
reponse has been processed by server. In this case, the
`res.setTimeout()` callback will never be called causing the test to
fail. Fix this by only closing the connection and server when the 2nd
has been received.

PR-URL: #11790
Fixes: #11768
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
Merged
@tniessen
Copy link
Member

This might still be flaky, I currently don't have time to investigate (node-test-binary-arm#8941):

not ok 98 parallel/test-http-set-timeout-server
  ---
  duration_ms: 11.210
  severity: fail
  stack: |-
    Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
        at Object.exports.mustCall (/home/iojs/build/workspace/node-test-binary-arm/test/common/index.js:486:10)
        at serverTimeout (/home/iojs/build/workspace/node-test-binary-arm/test/parallel/test-http-set-timeout-server.js:45:43)
        at /home/iojs/build/workspace/node-test-binary-arm/test/common/index.js:520:15
        at run (/home/iojs/build/workspace/node-test-binary-arm/test/parallel/test-http-set-timeout-server.js:40:5)
        at _combinedTickCallback (internal/process/next_tick.js:134:7)
        at process._tickCallback (internal/process/next_tick.js:201:9)
        at Function.Module.runMain (module.js:607:11)
        at startup (bootstrap_node.js:158:16)
        at bootstrap_node.js:575:3

andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@santigimeno @andrew749
test: fix flaky test-http-set-timeout-server
77d58bc 
It can happen that the connection and server is closed before the second
reponse has been processed by server. In this case, the
`res.setTimeout()` callback will never be called causing the test to
fail. Fix this by only closing the connection and server when the 2nd
has been received.

PR-URL: nodejs/node#11790
Fixes: nodejs/node#11768
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell left review comments

@Trott Trott Trott approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gibfahn gibfahn gibfahn approved these changes

Assignees
No one assigned
Labels
http Issues or PRs related to the http 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.

Investigate flaky parallel/test-http-set-timeout-server
9 participants
@santigimeno @gibfahn @tniessen @jasnell @Trott @cjihrig @MylesBorins @hiroppy @nodejs-github-bot