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

http: reset parser.incoming when server request is finished #29297

Closed
wants to merge 4 commits into from

Conversation

addaleax
Copy link
Member

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

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

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
@addaleax addaleax added the http Issues or PRs related to the http subsystem. label Aug 24, 2019
@addaleax addaleax requested a review from mcollina August 24, 2019 19:47
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member Author

Looks like the test fails on Windows sometimes. I’ll look into that, but if I don’t figure out what’s going on, I’d probably just add a skip() call to it.

lib/_http_server.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member Author

@mcollina I’ve pushed a commit to skip the test on Windows as suggested above. I’ll still investigate why, but this way this PR should be able to get into v12.9.1.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 26, 2019
@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented Aug 26, 2019

✖ This PR needs to wait 6 more hours to land

Can we please fast-track? Otherwise it won't be in 12.9.1.

@targos targos added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 26, 2019
@mcollina
Copy link
Member

Please 👍 for fast tracking it.

@addaleax
Copy link
Member Author

Landed in 6ab2848

addaleax added a commit that referenced this pull request Aug 26, 2019
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>
@addaleax addaleax closed this Aug 26, 2019
@addaleax addaleax deleted the redo-http-incoming branch August 26, 2019 13:22
targos pushed a commit that referenced this pull request Aug 26, 2019
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTPParser inhibits garbage collection on keep-alive connections
7 participants