Fix two missing async hooks destroy calls by basti1302 · Pull Request #23272 · nodejs/node

basti1302 · 2018-10-05T07:31:13Z

Supersedes #23201 and #23263 due to feedback from @addaleax in #23201.

Add a missing destroy for HTTP socket when the agent gets reused
Avoid duplicate call to AsyncWrap::Reset for HTTP parsers (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).
Add missing destroy for HTTP parsers when they are being reused.

Fixes: #19859

Reasoning: When calling asyncReset, we need to be aware that this will overwrite the AsyncWrap's asyncId, for which we might have emitted an init. The asyncId used for that init is then lost, thus, a destroy event will never be emitted for this "old" asyncId. The added emitDestroy makes sure that a matching destroy event is emitted before we assign the new asyncId in asyncReset.

Test cases have been added for all three items - all of these tests fail without the changes in production code.

Checklist

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

mcollina

LGTM with a nit.

mcollina · 2018-10-05T08:07:51Z

Can you make this check explicit? Turbofan prefers it to implicit boolean conversions.

Done. Whatever makes Turbofan happy :-)

basti1302 · 2018-10-05T08:48:42Z

nit adressed

addaleax · 2018-10-05T15:00:52Z

+void AsyncWrap::AsyncReset(double execution_async_id,
+ bool silent,
+ bool reused) {
+ if (reused) {


Would it be possible to initialize the async_id_ field on the class to e.g. -1 (or NaN if that doesn’t work)? That way we could tell here whether the instance is being re-used without explicitly having to tell it so, right?

🤦 - that's obviously possible, should have though of that. Thanks for spotting it, makes the code quite a bit simpler.
What we cannot get rid of however is the is_reused_symbol dance in freelist#alloc, because at that point, the HttpParser C++ object has been assigned an asyncId even if it is a freshly created instance (via its constructor calling the AsyncWrap super constructor).

basti1302 · 2018-10-06T08:37:21Z

I went ahead and squashed the commits to summarize the changes into one coherent commit message.

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. Fixes: nodejs#19859

basti1302 · 2018-10-08T19:03:02Z

Is anything else needed to land this?

mcollina · 2018-10-09T00:16:27Z

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

Should be good to go.

danbev · 2018-10-09T03:15:20Z

Re-run of failing node-test-commit-arm.

basti1302 · 2018-10-09T10:16:29Z

Seems the re-run of node-test-commit-arm was successful, so I guess the failed check node-test-commit does not block this.

danbev · 2018-10-10T06:51:45Z

Landed in eb9748d.

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: #23272Fixes: #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 · 2018-10-10T07:36:31Z

Thanks for landing this and in particular thanks to @addaleax and @mcollina for providing very constructive feedback. 🤗

I'll backport this to 10.x and 8.x next.

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#23272Fixes: 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>

targos · 2018-10-10T21:55:41Z

Should this be backported to v10.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

basti1302 · 2018-10-11T04:49:05Z

Hey @targos,

yes, as I said I'd like to backport this to both active LTS versions, that is, 10.x as well as 8.x. I was not aware of that guide, so I just had a look at other backport PRs and used them as a template. After reading the guide, I think I've done more or less exactly what is described in there.

The backport PRs are here:

[v8.x backport] async_hooks: add missing async_hooks destroys in AsyncReset #23410 (v8.x)
[v10.x backport] async_hooks: add missing async_hooks destroys in AsyncReset #23404 (v10.x)

I can't set labels on anything though, since I'm not a collaborator.

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: #23272Fixes: #19859 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

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#23272Fixes: 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>

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: #23404 PR-URL: #23272Fixes: #19859 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

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: #23272Fixes: #19859 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

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: #23404 PR-URL: #23272Fixes: #19859 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 5, 2018

This was referenced Oct 5, 2018
http: add missing async_hooks destroy before asyncReset #23201
Closed
http: remove duplicate async_hooks init for http parser #23263
Closed

mcollina reviewed Oct 5, 2018
View reviewed changes

Comment threadlib/internal/freelist.js Outdated
Comment threadlib/internal/freelist.js Outdated

basti1302 force-pushed the fix-missing-async-hooks-destroy-calls branch 2 times, most recently from bab65ec to 3d41d4fCompare October 5, 2018 07:52

mcollina approved these changes Oct 5, 2018
View reviewed changes

basti1302 force-pushed the fix-missing-async-hooks-destroy-calls branch from 0e1f49a to d1b552dCompare October 5, 2018 08:48

basti1302 force-pushed the fix-missing-async-hooks-destroy-calls branch 3 times, most recently from 11b0d99 to 5335c53Compare October 5, 2018 13:15

addaleax approved these changes Oct 5, 2018
View reviewed changes

basti1302 force-pushed the fix-missing-async-hooks-destroy-calls branch 2 times, most recently from 9da6d7a to 7652bd7Compare October 6, 2018 08:36

basti1302 force-pushed the fix-missing-async-hooks-destroy-calls branch from 7652bd7 to 9e3f644Compare October 6, 2018 08:40

basti1302 force-pushed the fix-missing-async-hooks-destroy-calls branch from 9e3f644 to 284a0e0Compare October 6, 2018 08:46

basti1302 mentioned this pull request Oct 6, 2018
Reusing HTTP connection lead to no destroy triggered #19859
Closed

mcollina added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 9, 2018

jasnell approved these changes Oct 9, 2018
View reviewed changes

danbev closed this Oct 10, 2018

basti1302 deleted the fix-missing-async-hooks-destroy-calls branch October 10, 2018 08:15

basti1302 mentioned this pull request Oct 10, 2018
[v10.x backport] async_hooks: add missing async_hooks destroys in AsyncReset #23404
Closed
3 tasks

This was referenced Oct 10, 2018
[v8.x backport] async_hooks: add missing async_hooks destroys in AsyncReset #23410
Closed
test: correct isReused parameter in test-http-parser #23412
Closed

targos added the backport-requested-v10.x label Oct 10, 2018

addaleax added backported-to-v10.x and removed backport-requested-v10.x labels Oct 11, 2018

BethGriggs mentioned this pull request Nov 12, 2018
v8.13.0 proposal #23974
Merged

beevelop mentioned this pull request Nov 20, 2018
2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs beevelop/docker-android-nodejs#250
Closed

codebytere mentioned this pull request Nov 27, 2018
v10.13.1 proposal #24675
Closed

codebytere mentioned this pull request Nov 29, 2018
v10.14.2 proposal #24727
Merged

watson mentioned this pull request Jan 15, 2019
High Memory issue at version 2.6.0 elastic/apm-agent-nodejs#772
Closed

Uh oh!

Uh oh!

Conversation

basti1302 commented Oct 5, 2018

Checklist

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollinaOct 5, 2018

Choose a reason for hiding this comment

Uh oh!

basti1302Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

basti1302 commented Oct 5, 2018

Uh oh!

addaleaxOct 5, 2018

Choose a reason for hiding this comment

Uh oh!

basti1302Oct 6, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

basti1302 commented Oct 6, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

basti1302 commented Oct 8, 2018

Uh oh!

mcollina commented Oct 9, 2018

Uh oh!

danbev commented Oct 9, 2018

Uh oh!

basti1302 commented Oct 9, 2018

Uh oh!

danbev commented Oct 10, 2018

Uh oh!

basti1302 commented Oct 10, 2018

Uh oh!

targos commented Oct 10, 2018

Uh oh!

basti1302 commented Oct 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

basti1302Oct 6, 2018•
edited
Loading

basti1302 commented Oct 6, 2018•
edited
Loading