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: make sure end is always emitted with keepalive connections #29263

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.
@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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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