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

test: fix flaky http-server-keep-alive-timeout #20093

Conversation

santigimeno
Copy link
Member

Make sure the connection is not closed until the 3 requests and the
timeout event are received.
Some code refactoring to avoid duplicated code.

Fixes: #20013

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Make sure the connection is not closed until the 3 requests and the
`timeout` event are received.
Some code refactoring to avoid duplicated code.

Fixes: nodejs#20013
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 17, 2018
@santigimeno
Copy link
Member Author

test(function serverEndKeepAliveTimeoutWithPipeline(cb) {
function done(server, socket, cb) {
socket.destroy();
server.close();
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I'd pass the callback to close() instead of calling it explicitly.

cb();
}

function server_test(with_pipeline, cb) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: use camel case.

@lpinca
Copy link
Member

lpinca commented Apr 17, 2018

Stress test on FreeBSD 10: https://ci.nodejs.org/job/node-stress-single-test/1818/

@apapirovski
Copy link
Member

apapirovski commented Apr 17, 2018

Doesn't look like it survived the stress test. 😞

Edit: I stopped the stress test since it was clearly not successful. Here are the results at that point in time:

12:52:05 + NOK=65
12:52:05 + echo '74   OK: 9   NOT OK: 65   TOTAL: 100'
12:52:05 74   OK: 9   NOT OK: 65   TOTAL: 100

@santigimeno
Copy link
Member Author

Yeah, the underlying issue seems to be that calling setTimeout() on a socket after it has been destroyed/closed works and I was not expecting that 🤷‍♂️

@santigimeno
Copy link
Member Author

Stress test should be: https://ci.nodejs.org/job/node-stress-single-test/1820/

Trott
Trott previously requested changes Apr 18, 2018
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.

Stress test failed. :-(

@santigimeno
Copy link
Member Author

@Trott yeah, but those failures are because of the stress tests params: -j 96 --repeat 192 are too much and the test sometimes times out. This new run with these params: -j 48 --repeat 96 passes correctly. I think the PR is ready.

@Trott Trott dismissed their stale review April 18, 2018 19:07

scaled-down stress test passes

server.close(cb);
}

function serverTest(with_pipeline, cb) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: there are still a couple of snake case variables. Would you mind fixing those before of when landing? Thanks.

santigimeno added a commit that referenced this pull request Apr 20, 2018
Make sure the connection is not closed until the 3 requests and the
`timeout` event are received.
Some code refactoring to avoid duplicated code.

Fixes: #20013
PR-URL: #20093
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@santigimeno
Copy link
Member Author

Landed in 998e174 with nit addressed. Thanks!

jasnell pushed a commit that referenced this pull request Apr 20, 2018
Make sure the connection is not closed until the 3 requests and the
`timeout` event are received.
Some code refactoring to avoid duplicated code.

Fixes: #20013
PR-URL: #20093
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate flaky parallel/test-http-server-keep-alive-timeout
7 participants