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

[v8.x backport] async_hooks: add missing async_hooks destroys in AsyncReset #23410

Closed
wants to merge 1 commit into from

Conversation

basti1302
Copy link

This adds missing async_hooks destroy calls for sockets (in
_http_agent.js) and HTTP parsers. We need to emit a destroy in
AsyncWrap#AsyncReset before assigning a new async_id when the instance
has already been in use and is being recycled, because in that case, we
have already emitted an init for the "old" async_id.

This also removes a duplicated init call for HTTP parser: Each time a
new parser was created, AsyncReset was being called via the C++ Parser
class constructor (super constructor AsyncWrap) and also via
Parser::Reinitialize.

PR-URL: #23272
Fixes: #19859
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: James M Snell jasnell@gmail.com

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

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v8.x labels Oct 10, 2018
@@ -464,6 +464,9 @@ class Parser : public AsyncWrap {
static void Reinitialize(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

CHECK(args[0]->IsInt32());
Copy link
Author

Choose a reason for hiding this comment

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

This check is actually new for the v8.x branch but since I was adding a new param here anyway (args[1]/isReused) I figured it wouldn't hurt to copy the existing check from master here.

@addaleax
Copy link
Member

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

As a heads up, this won’t be released in v8.x until #23272 has been in a Current release for 2 weeks, unless this is deemed a critical bugfix (which I assume it won’t be).

@basti1302
Copy link
Author

basti1302 commented Oct 11, 2018

As a heads up, this won’t be released in v8.x until #23272 has been in a Current release for 2 weeks, [...]

Just for my understanding - "Current release" in this case means a 10.x release, right? So the timeline would be

Is that how this works?

As to the criticality: I don't know the criteria for critical bugfixes. The issues causes memory leaks and thus causes processes to die with out-out-of-memory. At least customers of some APM vendors (like my company) are affected.

@BethGriggs BethGriggs added the baking-for-lts PRs that need to wait before landing in a LTS release. label Oct 16, 2018
This adds missing async_hooks destroy calls for sockets (in
_http_agent.js) and HTTP parsers. We need to emit a destroy in
AsyncWrap#AsyncReset before assigning a new async_id when the instance
has already been in use and is being recycled, because in that case, we
have already emitted an init for the "old" async_id.

This also removes a duplicated init call for HTTP parser: Each time a
new parser was created, AsyncReset was being called via the C++ Parser
class constructor (super constructor AsyncWrap) and also via
Parser::Reinitialize.

PR-URL: nodejs#23272
Fixes: nodejs#19859
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@basti1302
Copy link
Author

Rebased this on v8.x-staging to resolve conflict in src/async_wrap.cc.

MylesBorins pushed a commit that referenced this pull request Nov 4, 2018
This adds missing async_hooks destroy calls for sockets (in
_http_agent.js) and HTTP parsers. We need to emit a destroy in
AsyncWrap#AsyncReset before assigning a new async_id when the instance
has already been in use and is being recycled, because in that case, we
have already emitted an init for the "old" async_id.

This also removes a duplicated init call for HTTP parser: Each time a
new parser was created, AsyncReset was being called via the C++ Parser
class constructor (super constructor AsyncWrap) and also via
Parser::Reinitialize.

Backport-PR-URL: #23410
PR-URL: #23272
Fixes: #19859
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

landed in 26d145a

@MylesBorins MylesBorins closed this Nov 4, 2018
@MylesBorins MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Nov 4, 2018
@basti1302 basti1302 deleted the backport-8-23272 branch November 4, 2018 13:54
@basti1302
Copy link
Author

@BethGriggs Any chance this could be included in v8.13.0, that seems to be scheduled for later this month? I'm not familiar with the LTS release process, but #23974 looks like the scope of 8.13.0 is already being defined and this is probably not included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants