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

Fix two missing async hooks destroy calls #23272

Conversation

basti1302
Copy link

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

  1. Add a missing destroy for HTTP socket when the agent gets reused
  2. 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).
  3. 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

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 5, 2018
lib/internal/freelist.js Outdated Show resolved Hide resolved
lib/internal/freelist.js Show resolved Hide resolved
@basti1302 basti1302 force-pushed the fix-missing-async-hooks-destroy-calls branch 2 times, most recently from bab65ec to 3d41d4f Compare October 5, 2018 07:52
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 with a nit.

this.list.pop() :
this.ctor.apply(this, arguments);
let item;
if (this.list.length) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Done. Whatever makes Turbofan happy :-)

@basti1302 basti1302 force-pushed the fix-missing-async-hooks-destroy-calls branch from 0e1f49a to d1b552d Compare October 5, 2018 08:48
@basti1302
Copy link
Author

nit adressed

@basti1302 basti1302 force-pushed the fix-missing-async-hooks-destroy-calls branch 3 times, most recently from 11b0d99 to 5335c53 Compare October 5, 2018 13:15
void AsyncWrap::AsyncReset(double execution_async_id,
bool silent,
bool reused) {
if (reused) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

@basti1302 basti1302 Oct 6, 2018

Choose a reason for hiding this comment

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

🤦 - 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 basti1302 force-pushed the fix-missing-async-hooks-destroy-calls branch 2 times, most recently from 9da6d7a to 7652bd7 Compare October 6, 2018 08:36
@basti1302
Copy link
Author

basti1302 commented Oct 6, 2018

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

@basti1302 basti1302 force-pushed the fix-missing-async-hooks-destroy-calls branch from 7652bd7 to 9e3f644 Compare October 6, 2018 08:40
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
Copy link
Author

Is anything else needed to land this?

@mcollina
Copy link
Member

mcollina commented Oct 9, 2018

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

Should be good to go.

@mcollina 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
@danbev
Copy link
Member

danbev commented Oct 9, 2018

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

@basti1302
Copy link
Author

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
Copy link
Member

danbev commented Oct 10, 2018

Landed in eb9748d.

@danbev danbev closed this Oct 10, 2018
danbev pushed a commit that referenced this pull request Oct 10, 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: #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>
@basti1302
Copy link
Author

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.

@basti1302 basti1302 deleted the fix-missing-async-hooks-destroy-calls branch October 10, 2018 08:15
basti1302 added a commit to basti1302/node that referenced this pull request Oct 10, 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>
@targos
Copy link
Member

targos commented Oct 10, 2018

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
Copy link
Author

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:

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

@targos targos added this to Backported in v10.x Oct 16, 2018
jasnell pushed a commit that referenced this pull request Oct 17, 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: #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>
basti1302 added a commit to basti1302/node that referenced this pull request Nov 2, 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>
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: #23404
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 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>
@BethGriggs BethGriggs mentioned this pull request Nov 12, 2018
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 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: #23404
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 pushed a commit that referenced this pull request Nov 29, 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: #23404
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>
@codebytere codebytere mentioned this pull request Nov 29, 2018
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. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
No open projects
v10.x
  
Backported
Development

Successfully merging this pull request may close these issues.

Reusing HTTP connection lead to no destroy triggered
7 participants