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 http-upgrade-agent flakiness #4520

Closed

Conversation

santigimeno
Copy link
Member

It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the data event to make sure all
the data is received. Modify the test so it does not pass without
applying this change.

I was getting this error from time to time on OS X:

assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: <Buffer > == 'nurtzo'
    at ClientRequest.<anonymous> (/Users/sgimeno/node/node/test/parallel/test-http-upgrade-agent.js:51:12)
    at emitThree (events.js:98:13)
    at ClientRequest.emit (events.js:176:7)
    at Socket.socketOnData (_http_client.js:342:11)
    at emitOne (events.js:78:13)
    at Socket.emit (events.js:170:7)
    at readableAddChunk (_stream_readable.js:146:16)
    at Socket.Readable.push (_stream_readable.js:110:10)
    at TCP.onread (net.js:523:20)

@Trott
Copy link
Member

Trott commented Jan 3, 2016

Modify the test so it does not pass without applying this change.

This PR only has a change to the test. Is this sentence in error or is there a missing modified file that needs to be added?

@brendanashworth brendanashworth added the test Issues and PRs related to the tests. label Jan 4, 2016
@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Jan 4, 2016
@santigimeno
Copy link
Member Author

Yes, that sentence was wrong. I rewrote the commit message. PTAL

@Trott
Copy link
Member

Trott commented Jan 8, 2016

The changes to the upgrade event handler looks good to me. I don't think we need the setTimeout() in the test setup. If you do want to force the data in two writes like that, maybe put it in a different test so we have one test that's straightforward and another test that's specific to the data definitely always arriving in two pieces?

@santigimeno
Copy link
Member Author

@Trott PR updated with your comments. Thanks

@Trott
Copy link
Member

Trott commented Jan 9, 2016

LGTM if CI is happy.

CI: https://ci.nodejs.org/job/node-test-commit/1664/

We need to get you onboarded so you can start these CI jobs yourself!

Minor nit that can be ignored if you want: I don't think we need the gotData variable and check in the new test since we're guaranteeing the check of the value of recvData which can't possibly be set correctly unless gotData is true. But it doesn't hurt anything either, I guess.

@Trott
Copy link
Member

Trott commented Jan 9, 2016

One other comment that you can ignore if you want: The same issue you're fixing here seems to afflict test-http-upgrade-client.js as well. Maybe fix that too, either here or in a separate PR? (I'm a fan of separate PR, but either way.)

@santigimeno
Copy link
Member Author

@Trott I've removed the gotData variable.
I'll fix the other test in a separate PR

@Trott
Copy link
Member

Trott commented Jan 9, 2016

Looks like the modified test fails (or is flaky?) on FreeBSD and Raspberry Pi:

@santigimeno
Copy link
Member Author

@Trott It seems the CI was started with the previous version of the PR: 8ac8915 (the one with gotData) and not the current one: santigimeno@d710d9c. Anyway, it's been helpful as it has shown that in those environments all the upgrade data was sent in only 1 chunk. I'll increase the timeout a little bit and maybe bring back the gotData variable as it has proven to be useful. What do you think?

@santigimeno
Copy link
Member Author

Nah, increasing the timeout won't work either as the client is closing the connection when it receives the upgrade request. I think I'm removing the test altogether as it may happen that the second write could fail due to a race condition with the client closing the connection.

@santigimeno
Copy link
Member Author

PR updated. I think this should work

@Trott
Copy link
Member

Trott commented Jan 10, 2016

One nit, but LGTM if CI doesn't uncover anything.

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

recvData += d;
});

socket.on('close', function() {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: As mscdex noted in the other PR that's similar to this, this callback might benefit from being wrapped in common.mustCall() to guarantee that it fires.

It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.
@santigimeno
Copy link
Member Author

PR updated. Thanks

@Trott
Copy link
Member

Trott commented Jan 11, 2016

@jasnell
Copy link
Member

jasnell commented Jan 12, 2016

LGTM

jasnell pushed a commit that referenced this pull request Jan 12, 2016
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: #4520
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Jan 12, 2016

Landed in 6018fa1

@jasnell jasnell closed this Jan 12, 2016
rvagg pushed a commit that referenced this pull request Jan 14, 2016
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: #4520
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: #4520
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: #4520
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: nodejs#4520
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 11, 2016
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: nodejs#4520
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: nodejs#4520
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants