Skip to content

Conversation

@Trott
Copy link
Member

test-http-server-consumed-timeout will fail if the host is sufficiently
loaded that a 25ms interval takes more than 200ms to be invoked. Skip
the test in that situation.

Fixes: #14312

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

test

test-http-server-consumed-timeout will fail if the host is sufficiently loaded that a 25ms interval takes more than 200ms to be invoked. Skip the test in that situation. Fixes: nodejs#14312
@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Sep 29, 2017
@Trott
Copy link
MemberAuthor

The robustness here can be checked by copying the test to test/parallel and invoking it with:

tools/test.py -j 96 --repeat 192 test/parallel/test-http-server-consumed-timeout.js 

On master, I get many many failures that way. With this change, no failures.

@Trott
Copy link
MemberAuthor

CI stress test against master showing infrequent but still non-zero failures even though the test is in sequential: https://ci.nodejs.org/job/node-stress-single-test/1427/nodes=rhel72-s390x/console

CI stress test against this pull request, hopefully will come back green: https://ci.nodejs.org/job/node-stress-single-test/1428/nodes=rhel72-s390x/console

@Trott
Copy link
MemberAuthor

@BridgeAR

Copy link
Contributor

@refackrefack left a comment

Choose a reason for hiding this comment

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

Just nits

common.mustNotCall('Request timeout should not fire'));
req.setTimeout(TIMEOUT,()=>{
if(!intervalWasInvoked)
common.skip('interval was not invoked quickly enough for test');
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: return common.skip() as to not confuse static analysers

constnow=Date.now();
console.log('diff is '+(now-time));
if(time<now-TIMEOUT)
common.skip('interval is not invoked quickly enough for test');
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: return common.skip() as to not confuse static analysers

// to be invoked, skip the test.
constnow=Date.now();
console.log('diff is '+(now-time));
if(time<now-TIMEOUT)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I find now - time > TIMEOUT more intuitive

@mscdexmscdex added the http Issues or PRs related to the http subsystem. label Sep 29, 2017
@Trott
Copy link
MemberAuthor

@Fishrock123
Copy link
Contributor

I'm curious - is this a first? A test that skips if it times out?

@refack
Copy link
Contributor

@Fishrock123 There is at least one similar case

consttO=setTimeout(()=>{
console.log(stderr);
child.kill('SIGKILL');
process.exit(1);
},15*1000);
tO.unref();
child.on('close',(code,signal)=>{
clearTimeout(tO);
if(common.isWindows){
assert.strictEqual(code,134);
assert.strictEqual(signal,null);
}else{
assert.strictEqual(code,null);
// most posix systems will show 'SIGABRT', but alpine34 does not
if(signal!=='SIGABRT'){
console.log(`parent recived signal ${signal}\nchild's stderr:`);
console.log(stderr);
process.exit(1);
}
assert.strictEqual(signal,'SIGABRT');
}
assert.strictEqual(stdout,'');
constfirstLineStderr=stderr.split(/[\r\n]+/g)[0].trim();
assert.strictEqual(firstLineStderr,'Error: test_callback_abort');
});
console.timeEnd('end case 3');

If macOS is not configured properly creating a core dump could timeout so we preemptively timeout

Trott added a commit to Trott/io.js that referenced this pull request Oct 4, 2017
test-http-server-consumed-timeout will fail if the host is sufficiently loaded that a 25ms interval takes more than 200ms to be invoked. Skip the test in that situation. PR-URL: nodejs#15688Fixes: nodejs#14312 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
@Trott
Copy link
MemberAuthor

Trott commented Oct 4, 2017

Landed in a3cd8ed

@TrottTrott closed this Oct 4, 2017
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 4, 2017
test-http-server-consumed-timeout will fail if the host is sufficiently loaded that a 25ms interval takes more than 200ms to be invoked. Skip the test in that situation. PR-URL: nodejs/node#15688Fixes: nodejs/node#14312 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 7, 2017
test-http-server-consumed-timeout will fail if the host is sufficiently loaded that a 25ms interval takes more than 200ms to be invoked. Skip the test in that situation. PR-URL: #15688Fixes: #14312 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Oct 7, 2017
MylesBorins pushed a commit that referenced this pull request Oct 11, 2017
test-http-server-consumed-timeout will fail if the host is sufficiently loaded that a 25ms interval takes more than 200ms to be invoked. Skip the test in that situation. PR-URL: #15688Fixes: #14312 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 17, 2017
test-http-server-consumed-timeout will fail if the host is sufficiently loaded that a 25ms interval takes more than 200ms to be invoked. Skip the test in that situation. PR-URL: #15688Fixes: #14312 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Oct 17, 2017
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
test-http-server-consumed-timeout will fail if the host is sufficiently loaded that a 25ms interval takes more than 200ms to be invoked. Skip the test in that situation. PR-URL: #15688Fixes: #14312 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Nov 3, 2017
@TrottTrott deleted the fix-14312 branch January 13, 2022 22:46
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

httpIssues or PRs related to the http subsystem.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

investigate flaky "sequential/test-http-server-consumed-timeout"

9 participants

@Trott@Fishrock123@refack@jasnell@BridgeAR@gibfahn@mscdex@MylesBorins@nodejs-github-bot