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: add missing async_hooks destroy before asyncReset #23201

Conversation

basti1302
Copy link

@basti1302 basti1302 commented Oct 1, 2018

Emit a destroy before calling asyncReset because otherwise the old
async_id never gets destroyed.

Refs: #19859

This fixes one of the two issues described in here: #19859 (comment), namely the missing destroy event when an http agent gets reused.

Reasoning: When calling asyncReset on the socket, we need to be aware that this will overwrite the socket's asyncId. The asyncId assigned to the socket before asyncReset 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.

See also: #23263

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 http Issues or PRs related to the http subsystem. label Oct 1, 2018
@basti1302
Copy link
Author

This also adds a test that fails without the change and passes when the fix is applied.

I'd like to bring up two questions for potential reviewers:

  • Would it be preferable to issue the destroy event from C++ (that is, from within src/async_wrap.cc, void AsyncWrap::AsyncReset(double execution_async_id, bool silent))? The issue I see with that is that this method is also used for initializing new resources, where there is no "old" asyncId for which we need to emit a destroy.
  • The test I added has a lot of code in common with test/parallel/test-async-hooks-http-agent.js - should I rather merge my assertion into that? I can certainly do that, I just kept em separate because I feel making bunch of assertions in one test case is a bit meh.

@AyushG3112
Copy link
Contributor

cc @nodejs/http @nodejs/async_hooks

Emit a destroy before calling asyncReset because otherwise the old
async_id never gets destroyed.

Refs: nodejs#19859
@basti1302 basti1302 force-pushed the fix-missing-async-hooks-destroy-http-agent branch from eef00f1 to 08fcb79 Compare October 4, 2018 20:45
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Would it be preferable to issue the destroy event from C++ (that is, from within src/async_wrap.cc, void AsyncWrap::AsyncReset(double execution_async_id, bool silent))?

I think that might be a good idea, yes. This issue doesn’t seem to be specific to HTTP, but rather generally applies to AsyncWrap instances that are to be reset?

The test I added has a lot of code in common with test/parallel/test-async-hooks-http-agent.js - should I rather merge my assertion into that?

That’s your call… adding assertions to existing tests isn’t a bad thing in any way, though.

@@ -167,6 +172,9 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
var socket = this.freeSockets[name].shift();
// Guard against an uninitialized or user supplied Socket.
if (socket._handle && typeof socket._handle.asyncReset === 'function') {
if (destroyHooksExist() && socket._handle.getAsyncId()) {
emitDestroy(socket._handle.getAsyncId());
Copy link
Member

Choose a reason for hiding this comment

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

If we do the destroy from JS, can we cache the result of getAsyncId()? Calls into C++ are, unfortunately, not the cheapest…

Copy link
Author

@basti1302 basti1302 Oct 5, 2018

Choose a reason for hiding this comment

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

This call no longer exists in #23272.

@addaleax addaleax added async_wrap async_hooks Issues and PRs related to the async hooks subsystem. labels Oct 4, 2018
@basti1302
Copy link
Author

@addaleax Thanks for providing feedback. You are right, calling the destroy from C++ is probably the better call. It also saves a few trips to C++. I merged this and #23263 together and moved the EmitDestroy into C++ for all cases.

Closing this in favor of #23272.

@basti1302 basti1302 closed this Oct 5, 2018
@basti1302
Copy link
Author

basti1302 commented Oct 5, 2018

For completeness sake:

That’s your call… adding assertions to existing tests isn’t a bad thing in any way, though.

Well opinions differ on that, there are a bunch of folks who insist that tests should always only assert one thing. If there are no strong opinions I would keep the tests separate.

@basti1302 basti1302 deleted the fix-missing-async-hooks-destroy-http-agent branch October 10, 2018 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants