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: don't check the # of chunks in test-http-1.0 #3961

Closed
wants to merge 1 commit into from

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.
@mscdex mscdex 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
Member Author

@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
Member Author

@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

indutny commented Nov 25, 2015

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

@indutny
Copy link
Member

indutny commented Nov 25, 2015

LGTM

1 similar comment
@JungMinu
Copy link
Member

JungMinu commented Dec 6, 2015

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 <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
@bnoordhuis
Copy link
Member

Landed in dde2012, thanks Santiago.

@bnoordhuis bnoordhuis closed this Dec 7, 2015
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 <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
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 <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
@jasnell jasnell 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 <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
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 <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
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 <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
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