Skip to content

http: make sure end is always emitted with keepalive connections #29263

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

http: make sure end is always emitted with keepalive connections #29263

wants to merge 2 commits into from

Conversation

mcollina
Copy link
Member

Unfortunately 779a05d introduced a regression in 12.8.0, making the autocannon test failing on Node 12. This PR reverts 779a05d and adds a test to ensure that we do not regress again.

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

@mcollina
Revert "http: reset parser.incoming when server response is finished"
0eb3ce3 
This reverts commit 779a05d.
@mcollina
test, http: add regression test for keepalive 'end' event
3bef00e 
This test covers a regression where 'end' was not emitted in the case
of keepalive requests without parsing the full body.
@mcollina mcollina requested review from addaleax and lpinca August 22, 2019 09:46
@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Aug 22, 2019
@mcollina mcollina mentioned this pull request Aug 22, 2019
Closed
2 tasks
@mcollina mcollina requested review from Trott and benjamingr August 22, 2019 09:51
@mcollina
Copy link
Member Author

cc @indutny

@nodejs-github-bot
Copy link
Collaborator

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

@lpinca
Copy link
Member

lpinca commented Aug 22, 2019

#9668 should be reopened if/when this is merged.

@nodejs-github-bot
Copy link
Collaborator

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

@mcollina mcollina added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 23, 2019
@Trott
Copy link
Member

Trott commented Aug 24, 2019

Landed in d5c3837...b3172f8

@Trott Trott closed this Aug 24, 2019
Trott pushed a commit that referenced this pull request Aug 24, 2019
@mcollina @Trott
Revert "http: reset parser.incoming when server response is finished"
a5d9288 
This reverts commit 779a05d.

PR-URL: #29263
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Trott pushed a commit that referenced this pull request Aug 24, 2019
@mcollina @Trott
test, http: add regression test for keepalive 'end' event
b3172f8 
This test covers a regression where 'end' was not emitted in the case
of keepalive requests without parsing the full body.

PR-URL: #29263
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@ronag
Copy link
Member

ronag commented Aug 24, 2019 •
edited
Loading

This is a bit strange to me. In resOnFinish we emit req.'close'. So essentially the "bug" here is that we emit req.'end' when req.'close' has already been emitted. I think we have a different bug here as well in relation to req.'close'. I'll see if I can make a test. EDIT: yep nxtedition#1

@addaleax
Copy link
Member

@ronag The issue is that in resOnFinish the request might not have ended as a readable stream yet. I’m opening a new PR to fix this properly.

(Aside, if there’s a regression with one of my PRs, feel free to @addaleax me because I’ve configured my inbox to give explicit mentions of my handledhigher priority over other messages.)

addaleax added a commit to addaleax/node that referenced this pull request Aug 24, 2019
@addaleax
http: reset parser.incoming when server request is finished
40a2a8e 
This resolves a memory leak for keep-alive connections and does not
regress in the way that 779a05d did by waiting for
the incoming request to be finished before releasing the
`parser.incoming` object.

Refs: nodejs#28646
Refs: nodejs#29263
Fixes: nodejs#9668
This was referenced Aug 24, 2019
Closed
Closed
targos pushed a commit that referenced this pull request Aug 26, 2019
@mcollina @targos
Revert "http: reset parser.incoming when server response is finished"
f8f8754 
This reverts commit 779a05d.

PR-URL: #29263
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Aug 26, 2019
@mcollina @targos
test, http: add regression test for keepalive 'end' event
3c7a1a9 
This test covers a regression where 'end' was not emitted in the case
of keepalive requests without parsing the full body.

PR-URL: #29263
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos added a commit that referenced this pull request Aug 26, 2019
@targos
2019-08-26, Version 12.9.1 (Current)
0c2b98f 
Notable changes:

This release fixes two regressions in the http module:

* Fixes an event listener leak in the HTTP client. This resulted in lots
  of warnings during npm/yarn installs.
  #29245
* Fixes a regression preventing the `'end'` event from being emitted for
  keepalive requests in case the full body was not parsed.
  #29263

PR-URL: #29321
@targos targos mentioned this pull request Aug 26, 2019
Merged
@mcollina mcollina deleted the fix-autocannon branch August 26, 2019 10:11
addaleax added a commit that referenced this pull request Aug 26, 2019
@addaleax
http: reset parser.incoming when server request is finished
6ab2848 
This resolves a memory leak for keep-alive connections and does not
regress in the way that 779a05d did by waiting for
the incoming request to be finished before releasing the
`parser.incoming` object.

Refs: #28646
Refs: #29263
Fixes: #9668

PR-URL: #29297
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Aug 26, 2019
@addaleax @targos
http: reset parser.incoming when server request is finished
ae0a0e9 
This resolves a memory leak for keep-alive connections and does not
regress in the way that 779a05d did by waiting for
the incoming request to be finished before releasing the
`parser.incoming` object.

Refs: #28646
Refs: #29263
Fixes: #9668

PR-URL: #29297
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos added a commit that referenced this pull request Aug 26, 2019
@targos
2019-08-26, Version 12.9.1 (Current)
9cfb384 
Notable changes:

This release fixes two regressions in the http module:

* Fixes an event listener leak in the HTTP client. This resulted in lots
  of warnings during npm/yarn installs.
  #29245
* Fixes a regression preventing the `'end'` event from being emitted for
  keepalive requests in case the full body was not parsed.
  #29263

PR-URL: #29321
targos added a commit that referenced this pull request Aug 26, 2019
@targos
2019-08-26, Version 12.9.1 (Current)
d770b8a 
Notable changes:

This release fixes two regressions in the http module:

* Fixes an event listener leak in the HTTP client. This resulted in lots
  of warnings during npm/yarn installs.
  #29245
* Fixes a regression preventing the `'end'` event from being emitted for
  keepalive requests in case the full body was not parsed.
  #29263

PR-URL: #29321
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax Awaiting requested review from addaleax

@Trott Trott Awaiting requested review from Trott

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
8 participants
@mcollina @nodejs-github-bot @lpinca @Trott @ronag @addaleax @jasnell @benjamingr