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.

Sign up for GitHub

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

Jump to bottom

http: wait for both prefinish/end to keepalive #7149

Closed
wants to merge 1 commit into from
Closed

http: wait for both prefinish/end to keepalive #7149

wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Jun 4, 2016 •
edited
Loading

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)
Description of change

When freeing the socket to be reused in keep-alive Agent wait for both
prefinish and end events. Otherwise the next request may be written
before the previous one has finished sending the body, leading to a
parser errors.

R= @nodejs/http

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jun 4, 2016
@indutny
http: wait for both prefinish/end to keepalive
492bdde 
When `free`ing the socket to be reused in keep-alive Agent wait for both
`prefinish` and `end` events. Otherwise the next request may be written
before the previous one has finished sending the body, leading to a
parser errors.
@indutny indutny force-pushed the fix/agent-keepalive-on-pending branch from b2a1b5c to 492bdde Compare June 4, 2016 04:04
@indutny
Copy link
Member Author

indutny commented Jun 4, 2016

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

@indutny
Copy link
Member Author

indutny commented Jun 4, 2016

cc @nodejs/collaborators

@mcollina
Copy link
Member

mcollina commented Jun 4, 2016

LGTM

jasnell
jasnell reviewed Jun 6, 2016
View reviewed changes
test/parallel/test-http-client-keep-alive-release-before-finish.js
* `something`, and the client thought that this is a green light for sending
* next GET request
*/
post.write(Buffer.alloc(16 * 1024).fill('X'));
Copy link
Member

Choose a reason for hiding this comment

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

fwiw, this can be shortened to just Buffer.alloc(16 * 1024, 'X')

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

@jasnell
Copy link
Member

jasnell commented Jun 6, 2016

LGTM

@indutny
Copy link
Member Author

indutny commented Jun 6, 2016

Landed in 1004ece, thank you!

@indutny indutny closed this Jun 6, 2016
@indutny indutny deleted the fix/agent-keepalive-on-pending branch June 6, 2016 17:34
indutny added a commit that referenced this pull request Jun 6, 2016
@indutny
http: wait for both prefinish/end to keepalive
1004ece 
When `free`ing the socket to be reused in keep-alive Agent wait for
both `prefinish` and `end` events. Otherwise the next request may be
written before the previous one has finished sending the body, leading
to a parser errors.

PR-URL: #7149
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request Jun 15, 2016
@indutny @evanlucas
http: wait for both prefinish/end to keepalive
40e49de 
When `free`ing the socket to be reused in keep-alive Agent wait for
both `prefinish` and `end` events. Otherwise the next request may be
written before the previous one has finished sending the body, leading
to a parser errors.

PR-URL: #7149
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request Jun 16, 2016
@evanlucas
2016-06-16, Version 6.2.2 (Current)
2c50f3f 
Notable changes:

* **http**:
  - When maybeReadMore kicks in on a first bytes of incoming data, the
    req.read(0) will be invoked and the `req._consuming` will be set to
    true. This seemingly harmless property leads to a dire consequences:
    the server won't call `req._dump()` and the whole HTTP/1.1 pipeline
    will hang (single connection). (Fedor Indutny) [#7211](#7211)
  - When freeing the socket to be reused in keep-alive Agent wait for
    both prefinish and end events. Otherwise the next request may be
    written before the previous one has finished sending the body, leading
    to a parser errors. (Fedor Indutny) [#7149](#7149)
* **npm**: upgrade npm to 3.9.5 (Kat Marchán) [#7139](#7139)
@evanlucas evanlucas mentioned this pull request Jun 16, 2016
Merged
evanlucas added a commit that referenced this pull request Jun 16, 2016
@evanlucas
2016-06-16, Version 6.2.2 (Current)
8c43437 
Notable changes:

* **http**:
  - req.read(0) could cause incoming connections to stall and time out
    under certain conditions. (Fedor Indutny) [#7211](#7211)
  - When freeing the socket to be reused in keep-alive Agent wait for
    both prefinish and end events. Otherwise the next request may be
    written before the previous one has finished sending the body, leading
    to a parser errors. (Fedor Indutny) [#7149](#7149)
* **npm**: upgrade npm to 3.9.5 (Kat Marchán) [#7139](#7139)

#7323
evanlucas added a commit that referenced this pull request Jun 17, 2016
@evanlucas
2016-06-17, Version 6.2.2 (Current) Release
091abb3 
Notable changes:

* **http**:
  - req.read(0) could cause incoming connections to stall and time out
    under certain conditions. (Fedor Indutny) [#7211](#7211)
  - When freeing the socket to be reused in keep-alive Agent wait for
    both prefinish and end events. Otherwise the next request may be
    written before the previous one has finished sending the body, leading
    to a parser errors. (Fedor Indutny) [#7149](#7149)
* **npm**: upgrade npm to 3.9.5 (Kat Marchán) [#7139](#7139)

#7323
evanlucas added a commit that referenced this pull request Jun 17, 2016
@evanlucas
2016-06-17, Version 6.2.2 (Current) Release
e427150 
Notable changes:

* **http**:
  - req.read(0) could cause incoming connections to stall and time out
    under certain conditions. (Fedor Indutny) [#7211](#7211)
  - When freeing the socket to be reused in keep-alive Agent wait for
    both prefinish and end events. Otherwise the next request may be
    written before the previous one has finished sending the body, leading
    to a parser errors. (Fedor Indutny) [#7149](#7149)
* **npm**: upgrade npm to 3.9.5 (Kat Marchán) [#7139](#7139)

PR-URL: #7323
@outsideris outsideris mentioned this pull request Jun 28, 2016
Merged
@MylesBorins MylesBorins mentioned this pull request Jul 11, 2016
Closed
@MylesBorins
Copy link
Contributor

@indutny lts?

@indutny
Copy link
Member Author

indutny commented Jul 11, 2016

@thealphanerd yes

MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
@indutny
http: wait for both prefinish/end to keepalive
4e41a3b 
When `free`ing the socket to be reused in keep-alive Agent wait for
both `prefinish` and `end` events. Otherwise the next request may be
written before the previous one has finished sending the body, leading
to a parser errors.

PR-URL: #7149
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@indutny @MylesBorins
http: wait for both prefinish/end to keepalive
a96c461 
When `free`ing the socket to be reused in keep-alive Agent wait for
both `prefinish` and `end` events. Otherwise the next request may be
written before the previous one has finished sending the body, leading
to a parser errors.

PR-URL: #7149
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@indutny
http: wait for both prefinish/end to keepalive
60c86d2 
When `free`ing the socket to be reused in keep-alive Agent wait for
both `prefinish` and `end` events. Otherwise the next request may be
written before the previous one has finished sending the body, leading
to a parser errors.

PR-URL: #7149
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@indutny
http: wait for both prefinish/end to keepalive
ce0a1fa 
When `free`ing the socket to be reused in keep-alive Agent wait for
both `prefinish` and `end` events. Otherwise the next request may be
written before the previous one has finished sending the body, leading
to a parser errors.

PR-URL: #7149
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@indutny
http: wait for both prefinish/end to keepalive
9d13337 
When `free`ing the socket to be reused in keep-alive Agent wait for
both `prefinish` and `end` events. Otherwise the next request may be
written before the previous one has finished sending the body, leading
to a parser errors.

PR-URL: #7149
Reviewed-By: Matteo Collina <matteo.collina@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
Reviewers
No reviews
Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@indutny @mcollina @jasnell @MylesBorins @nodejs-github-bot