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.

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

Investigate flaky test-https-set-timeout-server on Raspberry Pi #14133

Closed
Trott opened this issue Jul 8, 2017 · 6 comments
Closed

Investigate flaky test-https-set-timeout-server on Raspberry Pi #14133

Trott opened this issue Jul 8, 2017 · 6 comments
Labels
arm Issues and PRs related to the ARM platform. flaky-test Issues and PRs related to the tests with unstable failures on the CI. https Issues or PRs related to the https subsystem.

Comments

@Trott
Copy link
Member

Trott commented Jul 8, 2017

  • Version: v9.0.0-pre
  • Platform: pi1-raspbian-wheezy
  • Subsystem: test, https

https://ci.nodejs.org/job/node-test-binary-arm/9111/RUN_SUBSET=0,label=pi1-raspbian-wheezy/console

not ok 219 sequential/test-https-set-timeout-server
  ---
  duration_ms: 10.610
  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:484:10)
        at serverTimeout (/home/iojs/build/workspace/node-test-binary-arm/test/sequential/test-https-set-timeout-server.js:57:12)
        at /home/iojs/build/workspace/node-test-binary-arm/test/common/index.js:518:15
        at run (/home/iojs/build/workspace/node-test-binary-arm/test/sequential/test-https-set-timeout-server.js:50:5)
        at _combinedTickCallback (internal/process/next_tick.js:131:7)
        at process._tickCallback (internal/process/next_tick.js:180:9)
        at Function.Module.runMain (module.js:607:11)
        at startup (bootstrap_node.js:158:16)
        at bootstrap_node.js:575:3

@nodejs/testing @nodejs/http

@Trott Trott added arm Issues and PRs related to the ARM platform. flaky-test Issues and PRs related to the tests with unstable failures on the CI. https Issues or PRs related to the https subsystem. labels Jul 8, 2017
@Trott
Copy link
Member Author

Trott commented Jul 8, 2017

Problem might be solved by moving cb() in all the test cases to inside a callback for server.close() rather than invoking it immediately after server.close() is invoked. Same might have to happen in corresponding http test.

@Trott
Copy link
Member Author

Trott commented Jul 8, 2017

(There could also be a case for moving each test into its own test file to rule out side effects.)

@Trott
Copy link
Member Author

Trott commented Jul 8, 2017

Moving the test to parallel and running it under load reproduces the issue.

$ tools/test.py -j92 --repeat 92 test/parallel/test-https-set-timeout-server.js  
=== release test-https-set-timeout-server ===                    
Path: parallel/test-https-set-timeout-server
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/Users/trott/io.js/test/common/index.js:484:10)
    at serverTimeout (/Users/trott/io.js/test/parallel/test-https-set-timeout-server.js:57:12)
    at /Users/trott/io.js/test/common/index.js:518:15
    at run (/Users/trott/io.js/test/parallel/test-https-set-timeout-server.js:50:5)
    at _combinedTickCallback (internal/process/next_tick.js:131:7)
    at process._tickCallback (internal/process/next_tick.js:180:9)
    at Function.Module.runMain (module.js:607:11)
    at startup (bootstrap_node.js:158:16)
    at bootstrap_node.js:575:3
...

@Trott
Copy link
Member Author

Trott commented Jul 8, 2017

If I comment out the first and last test case, the test becomes reliable.

@Trott
Copy link
Member Author

Trott commented Jul 8, 2017

@nodejs/http Is the following a bug or is it expected behavior?

Given this code:

const https = require('https');
const server = https.createServer(
  {key: keyfile, cert: certfile},
  function connectionListener(req, res) {
    // just do nothing, we should get a timeout event.
  });
server.listen(() => {
  const s = server.setTimeout(50, (socket) => {
    socket.destroy();
    server.close();
  });
  https.get({
    port: server.address().port,
    rejectUnauthorized: false
  }).on('error', () => {});
});

... connectionListener() may not run. Normally, it would. But it's not guaranteed as under load, the socket hangup/ECONNRESET may happen before the listener handler.

Bug? Expected behavior? Incorrect observation on my part? Something else?

Trott added a commit to Trott/io.js that referenced this issue Jul 8, 2017
Because of a race condition, connection listener may not be invoked if
test is run under load. Remove `common.mustCall()` wrapper from the
listener. Move the test to `parallel` because it now works under load.
Make similar change to http test to keep them in synch even though it is
much harder to trigger the race in http.

Fixes: nodejs#14133
@Trott
Copy link
Member Author

Trott commented Jul 8, 2017

On the assumption that the behavior described in the above comment is Not A Bug, I've opened #14134.

@Trott Trott closed this as completed in b20a0b6 Jul 11, 2017
addaleax pushed a commit that referenced this issue Jul 12, 2017
Because of a race condition, connection listener may not be invoked if
test is run under load. Remove `common.mustCall()` wrapper from the
listener. Move the test to `parallel` because it now works under load.
Make similar change to http test to keep them in synch even though it is
much harder to trigger the race in http.

PR-URL: #14134
Fixes: #14133
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
addaleax pushed a commit that referenced this issue Jul 18, 2017
Because of a race condition, connection listener may not be invoked if
test is run under load. Remove `common.mustCall()` wrapper from the
listener. Move the test to `parallel` because it now works under load.
Make similar change to http test to keep them in synch even though it is
much harder to trigger the race in http.

PR-URL: #14134
Fixes: #14133
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Fishrock123 pushed a commit that referenced this issue Jul 19, 2017
Because of a race condition, connection listener may not be invoked if
test is run under load. Remove `common.mustCall()` wrapper from the
listener. Move the test to `parallel` because it now works under load.
Make similar change to http test to keep them in synch even though it is
much harder to trigger the race in http.

PR-URL: #14134
Fixes: #14133
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 15, 2017
Because of a race condition, connection listener may not be invoked if
test is run under load. Remove `common.mustCall()` wrapper from the
listener. Move the test to `parallel` because it now works under load.
Make similar change to http test to keep them in synch even though it is
much harder to trigger the race in http.

PR-URL: #14134
Fixes: #14133
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 15, 2017
Because of a race condition, connection listener may not be invoked if
test is run under load. Remove `common.mustCall()` wrapper from the
listener. Move the test to `parallel` because it now works under load.
Make similar change to http test to keep them in synch even though it is
much harder to trigger the race in http.

PR-URL: #14134
Fixes: #14133
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 16, 2017
Because of a race condition, connection listener may not be invoked if
test is run under load. Remove `common.mustCall()` wrapper from the
listener. Move the test to `parallel` because it now works under load.
Make similar change to http test to keep them in synch even though it is
much harder to trigger the race in http.

PR-URL: #14134
Fixes: #14133
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 5, 2017
Because of a race condition, connection listener may not be invoked if
test is run under load. Remove `common.mustCall()` wrapper from the
listener. Move the test to `parallel` because it now works under load.
Make similar change to http test to keep them in synch even though it is
much harder to trigger the race in http.

PR-URL: #14134
Fixes: #14133
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arm Issues and PRs related to the ARM platform. flaky-test Issues and PRs related to the tests with unstable failures on the CI. https Issues or PRs related to the https subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant