Skip to content

Conversation

@santigimeno
Copy link
Member

As it can happen that the HTTP response is received in more than
one TCP chunk.

It tries to fix the following failure I got in a OS X box

assert.js:90 throw new assert.AssertionError({^ AssertionError: 1 == 2 at response_validator (/Users/sgimeno/node/node/test/parallel/test-http-1.0.js:111:12) at Socket.<anonymous> (/Users/sgimeno/node/node/test/parallel/test-http-1.0.js:49:7) at emitNone (events.js:73:20) at Socket.emit (events.js:167:7) at endReadableNT (_stream_readable.js:906:12) at doNTCallback2 (node.js:452:9) at process._tickCallback (node.js:366:17) 

Not to sure if this is the correct fix as I don't really understand the reason to check the number of chunks in this test.

As it can happen that the HTTP response is received in more than one TCP chunk.
@mscdexmscdex added http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. labels Nov 21, 2015
@bnoordhuis
Copy link
Member

I've never seen that test fail on OS X. What does the second chunk contain?

@santigimeno
Copy link
MemberAuthor

@bnoordhuis It only happened once so I can't really say. I could reproduce it later on increasing the size of the response but that's not the same test. I'll run the tests some more time during the following days and come back with the results

@santigimeno
Copy link
MemberAuthor

@bnoordhuis finally I could reproduce it.
The first chunk contains:

HTTP/1.1 200 OK Content-Type: text/plain Connection: close Hello, 

The second chunk:

world! 

@bnoordhuis
Copy link
Member

Huh, thanks. I figured out why it happens. There are a couple of places where the test does this:

res.write('Hello, ');res._send('');res.write('world!');res._send('');

Which results in this:

write(19, "HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nConnection: close\r\n\r\nHello, ", 71) = 71 write(19, "world!", 6) = 6 

But ends up getting read as a single packet, most of the time anyway.

read(16, "HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nConnection: close\r\n\r\nHello, world!", 65536) = 77 

I wonder if this isn't a performance issue (albeit a minor one) that we can fix by trying harder to coalesce writes. /cc @indutny - I know you were looking into a similar issue recently.

Back to this PR, LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/851/

@indutny
Copy link
Member

Nah, this is not that much related to it. I think.

@indutny
Copy link
Member

LGTM

1 similar comment
@JungMinu
Copy link
Member

LGTM

bnoordhuis pushed a commit that referenced this pull request Dec 7, 2015
As it can happen that the HTTP response is received in more than one TCP chunk. PR-URL: #3961 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
@bnoordhuis
Copy link
Member

Landed in dde2012, thanks Santiago.

rvagg pushed a commit that referenced this pull request Dec 8, 2015
As it can happen that the HTTP response is received in more than one TCP chunk. PR-URL: #3961 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 15, 2015
As it can happen that the HTTP response is received in more than one TCP chunk. PR-URL: #3961 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
@jasnelljasnell mentioned this pull request Dec 17, 2015
jasnell pushed a commit that referenced this pull request Dec 17, 2015
As it can happen that the HTTP response is received in more than one TCP chunk. PR-URL: #3961 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
As it can happen that the HTTP response is received in more than one TCP chunk. PR-URL: #3961 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
As it can happen that the HTTP response is received in more than one TCP chunk. PR-URL: nodejs#3961 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

httpIssues or PRs related to the http subsystem.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@santigimeno@bnoordhuis@indutny@JungMinu@mscdex@MylesBorins