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

async_hooks: don't reuse resource in HttpAgent #27581

Closed

Conversation

@Flarna
Copy link
Member

commented May 6, 2019

As discussed in nodejs/diagnostics#248, #21313 and https://docs.google.com/document/d/1g8OrG5lMIUhRn1zbkutgY83MiTSMx-0NHDs8Bf-nXxM/preview reusing the resource object is a blocker for landing a resource based async hooks API and get rid of the promise destroy hook.

This PR ensures that HttpAgent uses the a new resource object in case the socket handle gets reused.

fyi @nodejs/diagnostics

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
async_hooks: don't reuse resource in HttpAgent
As discussed in nodejs/diagnostics#248,
#21313 and
https://docs.google.com/document/d/1g8OrG5lMIUhRn1zbkutgY83MiTSMx-0NHDs8Bf-nXxM/preview
reusing the resource object is a blocker for landing a resource based
async hooks API and get rid of the promise destroy hook.

This PR ensures that HttpAgent uses the a new resource object in case
the socket handle gets reused.
@BridgeAR

This comment has been minimized.

Copy link
Member

commented May 13, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented May 13, 2019

I'll review this if I have to, but that's going to largely be a rubber-stamp review, and I'd prefer someone better-suited to review it do so. @nodejs/diagnostics @nodejs/http

@Flarna

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Maybe @nodejs/async_hooks should take a look as in the end it's a preparation for a resource based solution proposed by @mcollina in #21313.

@mcollina
Copy link
Member

left a comment

LGTM on green CI.

@nodejs-github-bot

This comment has been minimized.

@Trott
Trott approved these changes May 14, 2019
Copy link
Member

left a comment

Some minor suggestions for the test, but the test LGTM with or without the suggestions.

@Trott Trott added the author ready label May 14, 2019

@nodejs-github-bot

This comment has been minimized.

@Flarna

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

CI fail looks like #27617

@nodejs-github-bot

This comment has been minimized.

args[0]->IsNumber() ? args[0].As<Number>()->Value() : kInvalidAsyncId;
wrap->AsyncReset(execution_async_id);
args[1]->IsNumber() ? args[1].As<Number>()->Value() : kInvalidAsyncId;
wrap->AsyncReset(resource, execution_async_id);
}

This comment has been minimized.

Copy link
@addaleax

addaleax May 19, 2019

Member

This doesn’t have to stop this from landing, but thinking out loud a bit … I think we don’t have to deviate from the original async_hooks design this strongly, at least as far as the 1:1 correspondence of native objects and JS objects is concerned (which also has the advantage of having the resource objects provide information about the resource itself, which this PR undoes).

I think ideally what we would want for these cases (and maybe others) is to make the resource object passed to async_hooks refer to the native object, and give the native object a property over which we can figure out the current async_hooks resource object assoicated with it.

This comment has been minimized.

Copy link
@Flarna

Flarna May 19, 2019

Author Member

As far as I know for promises the resource passed via AsyncHooks is not linking to the Promise object itself at this time. So at least there we deviate already.

To my understanding passing the native resource would avoid to implement currentResource (#21313) as long as they are reused as setting a property on them or use them in a WeakMap would result in leak (except if the property gets removed via delete hook but this is the hook we want to get rid of).

I fully agree that it is not optimal to use different object types as resources depending on reuse or not.

My idea in this is different but not sure if it can implemented efficiently: Instead of passing the native resource object itself (which is often internal and not well documented, sometimes partly initialized,...) we could pass a well defined object for all resources. This object offers an APIs related to the current executing resource and even more related to scheduling (a name I have seen in other languages: ExecutionContext). With such an object in placed it's easier to extend the API as we are no longer bound to already existing low level objects.
If we consider the currently passed resource objects and types as part of the AsyncHooks API it's in my opinion more or less impossible to get it out of experimental as to much low level APIs are then part of the public API which would hinder refactorings,...

I could imagine that it would be also more efficient to think/signal more like a scheduler and issue only one executionChanged event with old and new context instead of a after event directly followed by a before event.

But again, this is just some idea I have in mind, not yet tried to implement it and not even sure if we get the needed information from v8/libuv. For sure this would be a breaking change.

This comment has been minimized.

Copy link
@addaleax

addaleax May 19, 2019

Member

My idea in this is different but not sure if it can implemented efficiently: Instead of passing the native resource object itself (which is often internal and not well documented, sometimes partly initialized,...) we could pass a well defined object for all resources. This object offers an APIs related to the current executing resource and even more related to scheduling (a name I have seen in other languages: ExecutionContext). With such an object in placed it's easier to extend the API as we are no longer bound to already existing low level objects.

@Flarna Yeah, I think we are in agreement here :)

@addaleax

This comment has been minimized.

Copy link
Member

commented May 19, 2019

Landed in 3d9d1ad

@addaleax addaleax closed this May 19, 2019

addaleax added a commit that referenced this pull request May 19, 2019
async_hooks: don't reuse resource in HttpAgent
As discussed in nodejs/diagnostics#248,
#21313 and
https://docs.google.com/document/d/1g8OrG5lMIUhRn1zbkutgY83MiTSMx-0NHDs8Bf-nXxM/preview
reusing the resource object is a blocker for landing a resource based
async hooks API and get rid of the promise destroy hook.

This PR ensures that HttpAgent uses the a new resource object in case
the socket handle gets reused.

PR-URL: #27581
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

@Flarna Flarna deleted the dynatrace-oss-contrib:async-hooks-reuse-http-agent branch May 19, 2019

targos added a commit that referenced this pull request May 20, 2019
async_hooks: don't reuse resource in HttpAgent
As discussed in nodejs/diagnostics#248,
#21313 and
https://docs.google.com/document/d/1g8OrG5lMIUhRn1zbkutgY83MiTSMx-0NHDs8Bf-nXxM/preview
reusing the resource object is a blocker for landing a resource based
async hooks API and get rid of the promise destroy hook.

This PR ensures that HttpAgent uses the a new resource object in case
the socket handle gets reused.

PR-URL: #27581
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR referenced this pull request May 21, 2019
4 of 4 tasks complete
@Flarna Flarna referenced this pull request Aug 7, 2019
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.