test: improve test-https-server-keep-alive-timeout #13312

Trott · 2017-05-30T20:12:32Z

The test is flaky under load. These changes greatly improve reliability.

Use a recurring interval to determine when the test should end rather
than a timer.
Increase server timeout to 500ms to allow for events being delayed by
system load

Changing to an interval has the added benefit of reducing the test run
time from over 2 seconds to under 1 second.

Fixes: #13307

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test https

kunalspathak · 2017-05-30T20:53:38Z

test/parallel/test-https-server-keep-alive-timeout.js

  }));
 });

 test(function serverNoEndKeepAliveTimeoutWithPipeline(cb) {
  let socket;
-  let destroyedSockets = 0;
  let timeoutCount = 0;
  let requestCount = 0;
  process.on('exit', () => {
    assert.strictEqual(timeoutCount, 1);
    assert.strictEqual(requestCount, 3);


assert.strictEqual(requestCount, 3); [](start = 4, length = 36)

Do we need this since you are clearing the interval with requestCount === 3 ?

I left it in to make sure it doesn't subsequently increase to 4 or higher. Probably not needed but also maybe doesn't hurt?

jasnell · 2017-05-30T23:01:16Z

test/parallel/test-https-server-keep-alive-timeout.js

@@ -31,20 +31,18 @@ function run() {

 test(function serverKeepAliveTimeoutWithPipeline(cb) {
  let socket;
-  let destroyedSockets = 0;
  let timeoutCount = 0;
  let requestCount = 0;
  process.on('exit', function() {
    assert.strictEqual(timeoutCount, 1);
    assert.strictEqual(requestCount, 3);


Can the test be refactored to eliminate the need for the exit handler?

Yes, but I'd rather that be in a separate PR.

I agree that since this PR is focused on robustness, it will be "offtopic" to remove, but now it "cries out" to remove this in favor of a mustCall(x,3) in line (new file) 37, and corresponding 63 & 66.

aqrln

Thanks!

lpinca · 2017-05-31T06:35:08Z

Maybe I'm missing something but why is the polling/interval needed? Isn't wrapping the callback of server.setTimeout() with common.mustCall() sufficient?

Trott · 2017-05-31T17:04:32Z

Maybe I'm missing something but why is the polling/interval needed? Isn't wrapping the callback of server.setTimeout() with common.mustCall() sufficient?

@lpinca I added an additional commit to remove the interval. Seems to work (including under load). LGTY?

lpinca · 2017-05-31T17:31:16Z

LGTM. It's a lot cleaner 👍 .

refack · 2017-05-31T20:38:52Z

LGTM. It's a lot cleaner 👍 .

Personally I like the setInterval(fn, 1) idiom (#13252 (comment)) since it's less arbitrary than 500, but not enough for asking another change 🤷‍♂️
I do think we should think about this... #13346 just so we don't drive contributors crazy with back and forth.

Trott · 2017-05-31T20:46:11Z

Personally I like the setInterval(fn, 1) idiom (#13252 (comment)) since it's less arbitrary than 500,

I don't think setInterval() can be applied here, or at least not trivially, because the 500 is not the timeout for a setTimeout() call but a server.setTimeout(). Unfortunate/confusing name, but there it is.

lpinca · 2017-05-31T20:52:27Z

What @Trott said. In this case the interval was cleared when the server.setTimeout() callback was fired making the whole interval useless imo.

Trott · 2017-05-31T20:55:28Z

(Oh, and the 500 ms server.setTimeout() has been there all along. It didn't replace the setInterval(). Just in case that's the source of confusion. In the existing test, it's a 200 ms timeout though.)

refack · 2017-05-31T21:03:33Z

Yeah ok, but still the setInterval had if (socket && socket.destroyed && requestCount === 3). Who counts to 3 now?
AFAICT the 500 is there to give the 3 req enough time to fire? So it is an "empirical" solution not an "analytical" one, right? On an extremely busy machine the 3 will not get to the server and the test will flake.

Again I'm not objecting, just thinking out loud. I'm fine with K.I.S.S.

Trott · 2017-06-01T04:11:18Z

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

lpinca · 2017-06-01T05:51:57Z

Yeah ok, but still the setInterval had if (socket && socket.destroyed && requestCount === 3). Who counts to 3 now?

I don't think this is relevant. As it was, the interval would have never been cleared if requestCount !== 3 making the test fail for timeout? The requestCount is still there and checked when process exits.

I wonder if that small value (50) for server.keepAliveTimeout is intended though.

refack

just nits

refack · 2017-06-01T13:54:01Z

test/parallel/test-https-server-keep-alive-timeout.js

@@ -31,20 +31,18 @@ function run() {

 test(function serverKeepAliveTimeoutWithPipeline(cb) {
  let socket;
-  let destroyedSockets = 0;
  let timeoutCount = 0;
  let requestCount = 0;
  process.on('exit', function() {
    assert.strictEqual(timeoutCount, 1);
    assert.strictEqual(requestCount, 3);


I agree that since this PR is focused on robustness, it will be "offtopic" to remove, but now it "cries out" to remove this in favor of a mustCall(x,3) in line (new file) 37, and corresponding 63 & 66.

refack · 2017-06-01T13:57:16Z

test/parallel/test-https-server-keep-alive-timeout.js

    socket.destroy();
-  });
+    server.close();


nit: just for readability, could you put comment that // this ends the test or do a:

function endTest(socket) { socket.destroy(); server.close(); cb(); } ... server.setTimeout(500, common.mustCall(endTest));

Here and in L41?

The test is flaky under load. These changes greatly improve reliability. * Use a recurring interval to determine when the test should end rather than a timer. * Increase server timeout to 500ms to allow for events being delayed by system load Changing to an interval has the added benefit of reducing the test run time from over 2 seconds to under 1 second. Fixes: nodejs#13307

Trott · 2017-06-01T18:22:46Z

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

The test is flaky under load. These changes greatly improve reliability. * Use a recurring interval to determine when the test should end rather than a timer. * Increase server timeout to 500ms to allow for events being delayed by system load Changing to an interval has the added benefit of reducing the test run time from over 2 seconds to under 1 second. Fixes: #13307 PR-URL: #13312 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

jasnell · 2017-06-01T22:52:37Z

Landed in ce5745b

The test is flaky under load. These changes greatly improve reliability. * Use a recurring interval to determine when the test should end rather than a timer. * Increase server timeout to 500ms to allow for events being delayed by system load Changing to an interval has the added benefit of reducing the test run time from over 2 seconds to under 1 second. Fixes: #13307 PR-URL: #13312 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

Make the same reliability changes that were applied to the https test in ce5745b. Refs: #13312 PR-URL: #13448 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

Trott added https Issues or PRs related to the https subsystem. test Issues and PRs related to the tests. labels May 30, 2017

Trott mentioned this pull request May 30, 2017

Investigate flaky test-https-server-keep-alive-timeout #13307

Closed

kunalspathak reviewed May 30, 2017

View reviewed changes

jasnell reviewed May 30, 2017

View reviewed changes

aqrln approved these changes May 30, 2017

View reviewed changes

refack approved these changes May 31, 2017

View reviewed changes

lpinca approved these changes May 31, 2017

View reviewed changes

refack mentioned this pull request May 31, 2017

question, test: best way to retry async task #13346

Closed

refack approved these changes May 31, 2017

View reviewed changes

refack approved these changes Jun 1, 2017

View reviewed changes

Trott added 3 commits June 1, 2017 11:13

squash: additional refactor per lpinca

80c1f44

squash: nits from refack

e89d8ca

Trott force-pushed the deflake-timeout branch from d536a33 to e89d8ca Compare June 1, 2017 18:16

jasnell approved these changes Jun 1, 2017

View reviewed changes

jasnell closed this Jun 1, 2017

gibfahn mentioned this pull request Jun 4, 2017

test: import test-http-server-keep-alive-timeout #13448

Closed

4 tasks

jasnell mentioned this pull request Jun 5, 2017

8.1.0 Proposal #13483

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

Trott deleted the deflake-timeout branch January 13, 2022 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: improve test-https-server-keep-alive-timeout #13312

test: improve test-https-server-keep-alive-timeout #13312

Trott commented May 30, 2017

kunalspathak May 30, 2017

Trott May 30, 2017

jasnell May 30, 2017

Trott May 30, 2017

refack Jun 1, 2017

aqrln left a comment

lpinca commented May 31, 2017 •

edited

Loading

Trott commented May 31, 2017

lpinca commented May 31, 2017 •

edited

Loading

refack commented May 31, 2017

Trott commented May 31, 2017

lpinca commented May 31, 2017 •

edited

Loading

Trott commented May 31, 2017

refack commented May 31, 2017

Trott commented Jun 1, 2017

lpinca commented Jun 1, 2017 •

edited

Loading

refack left a comment

refack Jun 1, 2017

refack Jun 1, 2017 •

edited

Loading

Trott Jun 1, 2017

Trott commented Jun 1, 2017

jasnell commented Jun 1, 2017

test: improve test-https-server-keep-alive-timeout #13312

test: improve test-https-server-keep-alive-timeout #13312

Conversation

Trott commented May 30, 2017

Checklist

Affected core subsystem(s)

kunalspathak May 30, 2017

Choose a reason for hiding this comment

Trott May 30, 2017

Choose a reason for hiding this comment

jasnell May 30, 2017

Choose a reason for hiding this comment

Trott May 30, 2017

Choose a reason for hiding this comment

refack Jun 1, 2017

Choose a reason for hiding this comment

aqrln left a comment

Choose a reason for hiding this comment

lpinca commented May 31, 2017 • edited Loading

Trott commented May 31, 2017

lpinca commented May 31, 2017 • edited Loading

refack commented May 31, 2017

Trott commented May 31, 2017

lpinca commented May 31, 2017 • edited Loading

Trott commented May 31, 2017

refack commented May 31, 2017

Trott commented Jun 1, 2017

lpinca commented Jun 1, 2017 • edited Loading

refack left a comment

Choose a reason for hiding this comment

refack Jun 1, 2017

Choose a reason for hiding this comment

refack Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

Trott Jun 1, 2017

Choose a reason for hiding this comment

Trott commented Jun 1, 2017

jasnell commented Jun 1, 2017

lpinca commented May 31, 2017 •

edited

Loading

lpinca commented May 31, 2017 •

edited

Loading

lpinca commented May 31, 2017 •

edited

Loading

lpinca commented Jun 1, 2017 •

edited

Loading

refack Jun 1, 2017 •

edited

Loading