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 parallel/test-http-set-timeout-server #11768

Closed
mscdex opened this issue Mar 9, 2017 · 9 comments · Fixed by #11790
Closed

Investigate flaky parallel/test-http-set-timeout-server #11768

mscdex opened this issue Mar 9, 2017 · 9 comments · Fixed by #11790
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. freebsd Issues and PRs related to the FreeBSD platform. http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.

Comments

@mscdex
Copy link
Contributor

mscdex commented Mar 9, 2017

  • Version: master
  • Platform: freebsd,pi1-raspbian-wheezy
  • Subsystem: http

Example: https://ci.nodejs.org/job/node-test-commit-freebsd/7525/nodes=freebsd10-64/console

not ok 612 parallel/test-http-set-timeout-server
  ---
  duration_ms: 1.315
  severity: fail
  stack: |-
    # serverTimeout
    # serverRequestTimeout
    # serverResponseTimeout
    # serverRequestNotTimeoutAfterEnd
    # serverResponseTimeoutWithPipeline
    # idleTimeout
    ok
    
    assert.js:81
      throw new assert.AssertionError({
      ^
    AssertionError: '' === '/2'
        at process.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/parallel/test-http-set-timeout-server.js:121:12)
        at emitOne (events.js:101:20)
        at process.emit (events.js:191:7)
@mscdex mscdex added freebsd Issues and PRs related to the FreeBSD platform. http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. labels Mar 9, 2017
santigimeno added a commit to santigimeno/node that referenced this issue Mar 14, 2017
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 pushed a commit to italoacasas/node that referenced this issue Mar 14, 2017
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>
jungx098 pushed a commit to jungx098/node that referenced this issue Mar 21, 2017
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 issue Apr 17, 2017
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 issue Apr 19, 2017
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>
@refack refack added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Jun 30, 2017
@refack
Copy link
Contributor

refack commented Jun 30, 2017

https://ci.nodejs.org/job/node-test-commit-freebsd/10036/nodes=freebsd10-64/

not ok 702 parallel/test-http-set-timeout-server
  ---
  duration_ms: 6.648
  severity: fail
  stack: |-
    Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
        at Object.exports.mustCall (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/common/index.js:486:10)
        at serverTimeout (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/parallel/test-http-set-timeout-server.js:45:43)
        at /usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/common/index.js:520:15
        at run (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/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

@refack refack reopened this Jun 30, 2017
@refack
Copy link
Contributor

refack commented Jul 4, 2017

Now on ARM (pi1-raspbian-wheezy):

not ok 99 parallel/test-http-set-timeout-server
  ---
  duration_ms: 8.803
  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/parallel/test-http-set-timeout-server.js:45:43)
        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/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
  ...

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

@refack refack changed the title Investigate flaky parallel/test-http-set-timeout-server on freebsd Investigate flaky parallel/test-http-set-timeout-server Jul 4, 2017
@refack refack added the arm Issues and PRs related to the ARM platform. label Jul 4, 2017
@Trott
Copy link
Member

Trott commented Jul 14, 2017

FreeBSD again:

https://ci.nodejs.org/job/node-test-commit-freebsd/10317/nodes=freebsd10-64/console

not ok 697 parallel/test-http-set-timeout-server
  ---
  duration_ms: 2.804
  severity: fail
  stack: |-
    Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
        at Object.exports.mustCall (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/common/index.js:481:10)
        at idleTimeout (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/parallel/test-http-set-timeout-server.js:155:43)
        at /usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/common/index.js:513:15
        at run (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/parallel/test-http-set-timeout-server.js:40:5)
        at Server.server.on.common.mustCall (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/parallel/test-http-set-timeout-server.js:138:7)
        at Server.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/common/index.js:513:15)
        at emitOne (events.js:115:13)
        at Server.emit (events.js:210:7)
        at Socket.socketOnTimeout (_http_server.js:392:35)

@Trott
Copy link
Member

Trott commented Jul 19, 2017

More FreeBSD:

https://ci.nodejs.org/job/node-test-commit-freebsd/10453/nodes=freebsd10-64/console

not ok 701 parallel/test-http-set-timeout-server
  ---
  duration_ms: 2.836
  severity: fail
  stack: |-
    Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
        at Object.exports.mustCall (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/common/index.js:480:10)
        at idleTimeout (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/parallel/test-http-set-timeout-server.js:155:43)
        at /usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/common/index.js:512:15
        at run (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/parallel/test-http-set-timeout-server.js:40:5)
        at Server.server.on.common.mustCall (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/parallel/test-http-set-timeout-server.js:138:7)
        at Server.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/common/index.js:512:15)
        at emitOne (events.js:115:13)
        at Server.emit (events.js:210:7)
        at Socket.socketOnTimeout (_http_server.js:392:35)

andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
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>
@Trott
Copy link
Member

Trott commented Jul 20, 2017

I can replicate this by reducing the timeout on line 160 to 1ms and running the test under load. Confirms what seems little obvious, which is that the test is still race-y.

@Trott
Copy link
Member

Trott commented Jul 20, 2017

Since being re-opened, the error has always been in line 155 which is not where the problem was before, which suggests that this is either a new problem or a problem only revealed by fixing the previous problem.

@Trott
Copy link
Member

Trott commented Jul 20, 2017

Fixing coming momentarily...

@Trott
Copy link
Member

Trott commented Jul 20, 2017

Proposed fix in #14380

Trott added a commit to Trott/io.js that referenced this issue Jul 24, 2017
The tests include a callback that might not be invoked but is wrapped in
common.mustCall(). Remove the common.mustCall() wrapper and add a
comment explaining that it should not be added.

Add a new test case that sets the timeout to 1ms and waits for both the
connection handler and the timeout handler to be invoked. This version
keeps the common.mustCall() wrapper intact around the connection handler
(although it's mostly semantic and not necessary for the test as the
test will certainly fail or time out if that handler isn't invoked).

Fixes: nodejs#11768
@Trott Trott closed this as completed in c5bed4c Jul 25, 2017
addaleax pushed a commit that referenced this issue Jul 27, 2017
The tests include a callback that might not be invoked but is wrapped in
common.mustCall(). Remove the common.mustCall() wrapper and add a
comment explaining that it should not be added.

Add a new test case that sets the timeout to 1ms and waits for both the
connection handler and the timeout handler to be invoked. This version
keeps the common.mustCall() wrapper intact around the connection handler
(although it's mostly semantic and not necessary for the test as the
test will certainly fail or time out if that handler isn't invoked).

PR-URL: #14380
Fixes: #11768
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 15, 2017
The tests include a callback that might not be invoked but is wrapped in
common.mustCall(). Remove the common.mustCall() wrapper and add a
comment explaining that it should not be added.

Add a new test case that sets the timeout to 1ms and waits for both the
connection handler and the timeout handler to be invoked. This version
keeps the common.mustCall() wrapper intact around the connection handler
(although it's mostly semantic and not necessary for the test as the
test will certainly fail or time out if that handler isn't invoked).

PR-URL: #14380
Fixes: #11768
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 15, 2017
The tests include a callback that might not be invoked but is wrapped in
common.mustCall(). Remove the common.mustCall() wrapper and add a
comment explaining that it should not be added.

Add a new test case that sets the timeout to 1ms and waits for both the
connection handler and the timeout handler to be invoked. This version
keeps the common.mustCall() wrapper intact around the connection handler
(although it's mostly semantic and not necessary for the test as the
test will certainly fail or time out if that handler isn't invoked).

PR-URL: #14380
Fixes: #11768
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 16, 2017
The tests include a callback that might not be invoked but is wrapped in
common.mustCall(). Remove the common.mustCall() wrapper and add a
comment explaining that it should not be added.

Add a new test case that sets the timeout to 1ms and waits for both the
connection handler and the timeout handler to be invoked. This version
keeps the common.mustCall() wrapper intact around the connection handler
(although it's mostly semantic and not necessary for the test as the
test will certainly fail or time out if that handler isn't invoked).

PR-URL: #14380
Fixes: #11768
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Sep 5, 2017
The tests include a callback that might not be invoked but is wrapped in
common.mustCall(). Remove the common.mustCall() wrapper and add a
comment explaining that it should not be added.

Add a new test case that sets the timeout to 1ms and waits for both the
connection handler and the timeout handler to be invoked. This version
keeps the common.mustCall() wrapper intact around the connection handler
(although it's mostly semantic and not necessary for the test as the
test will certainly fail or time out if that handler isn't invoked).

PR-URL: #14380
Fixes: #11768
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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. freebsd Issues and PRs related to the FreeBSD platform. http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
3 participants