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: unmerge resource_symbol from owner_symbol #40741

Conversation

RaisinTen
Copy link
Contributor

@RaisinTen RaisinTen commented Nov 6, 2021

This reverts 7ca2f13 and 937bbc5 and adds some regression tests for the referenced issue.

Fixes: #40693

Signed-off-by: Darshan Sen [email protected]

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. needs-ci PRs that need a full CI run. labels Nov 6, 2021
@nodejs-github-bot

This comment has been minimized.

@targos
Copy link
Member

targos commented Nov 6, 2021

@nodejs/diagnostics (btw @vdeturckheim would you like to be added to that team?)

@RaisinTen
Copy link
Contributor Author

cc @nodejs/async_hooks

@nodejs-github-bot

This comment has been minimized.

@vdeturckheim
Copy link
Member

@targos thanks for the heads up, I am in @nodejs/async_hooks but it probably makes sense for me to join @nodejs/diagnostics too 🤔

@targos
Copy link
Member

targos commented Nov 6, 2021

@vdeturckheim I don't see you in the async_hooks team either.

@Qard
Copy link
Member

Qard commented Nov 6, 2021

Seems I missed #38468...

IMO, resource_symbol should never have been removed in the first place. The resource stack logic needs a different object from what owner_symbol provides in many cases. Using the constructor name to conditionally trigger additional unwrapping seems a bit fragile to me. What if user code creates a resource with the same name? This can be achieved trivially by sub-classes AsyncResource.

Seems to me like it'd be best to revert #38468 and keep those separate, or maybe an owner hierarchy where the regular owner_symbol stuff just grabs the direct owner while executionAsyncResource(...) keeps following the links until it finds something without an owner?

@RaisinTen
Copy link
Contributor Author

RaisinTen commented Nov 7, 2021

@Qard

Seems I missed #38468...

IMO, resource_symbol should never have been removed in the first place. The resource stack logic needs a different object from what owner_symbol provides in many cases.

Could you please share some use cases that need the resource_symbol to be present even though the owner_symbol exists?

Using the constructor name to conditionally trigger additional unwrapping seems a bit fragile to me. What if user code creates a resource with the same name? This can be achieved trivially by sub-classes AsyncResource.

In that case, I think bea58a1 should be fine?

Seems to me like it'd be best to revert #38468 and keep those separate, or maybe an owner hierarchy where the regular owner_symbol stuff just grabs the direct owner while executionAsyncResource(...) keeps following the links until it finds something without an owner?

If I'm not wrong, currently the owner_symbol does indeed point to the direct owner of a resource. I've submitted cbb274b to let executionAsyncResource() return the topmost owner of a resource. With this fix, do we still need to revert #38468?

lib/async_hooks.js Outdated Show resolved Hide resolved
@RaisinTen RaisinTen force-pushed the use-correct-resource-for-asynclocalstorage branch from 5ab8518 to 12abaf7 Compare November 7, 2021 10:38
@RaisinTen RaisinTen requested a review from Flarna November 7, 2021 11:01
@addaleax
Copy link
Member

addaleax commented Nov 7, 2021

IMO, resource_symbol should never have been removed in the first place. The resource stack logic needs a different object from what owner_symbol provides in many cases.

Could you please share some use cases that need the resource_symbol to be present even though the owner_symbol exists?

I would also be curious about this – both resource_symbol and owner_symbol point to the public JS API object associated with the current resource that should be returned for diagnostic purposes. They have the same semantics.

@Qard
Copy link
Member

Qard commented Nov 7, 2021

Any sort of connection pooling needs an AsyncResource rather than the public API object to express the individual task, because sharing a resource across multiple tasks will corrupt the context. This happens in http.Agent and many places in userland code. This is why the resource was separate from owner in the first place. It could be nested on the owner though, but I feel like we would want something a bit less fragile than a bunch of constructor name checks which, as I said, can be broken by simply sub-classing AsyncResource, which a bunch of userland already does.

I do agree with the idea of simplifying this as much as we can, but what we're seeing here is that we simplified too much and broke the handle reuse code. We still need to handle that properly somehow, so I feel the removal of resource_symbol needs a bit more thought.

I won't block on this PR, because this needs fixing quickly, but I would request at least a comment with a big, bold WARNING about the safety of constructor name checking and a TODO to improve the handling of that.

@RaisinTen
Copy link
Contributor Author

RaisinTen commented Nov 11, 2021

@Qard

Any sort of connection pooling needs an AsyncResource rather than the public API object to express the individual task, because sharing a resource across multiple tasks will corrupt the context. This happens in http.Agent and many places in userland code. This is why the resource was separate from owner in the first place. It could be nested on the owner though, but I feel like we would want something a bit less fragile than a bunch of constructor name checks which, as I said, can be broken by simply sub-classing AsyncResource, which a bunch of userland already does.

I do agree with the idea of simplifying this as much as we can, but what we're seeing here is that we simplified too much and broke the handle reuse code. We still need to handle that properly somehow, so I feel the removal of resource_symbol needs a bit more thought.

If you're talking about handle reuse, I've added the tests back in 323f8a7, which I removed by mistake instead of fixing them in #38468, so I think it still works as expected.

I won't block on this PR, because this needs fixing quickly, but I would request at least a comment with a big, bold WARNING about the safety of constructor name checking and a TODO to improve the handling of that.

Wasn't this already addressed in bea58a1 or am I missing something here?

@Qard
Copy link
Member

Qard commented Nov 11, 2021

Yep, I had missed those changes when I wrote that comment as I had left the tab open for awhile to review before commenting and it had not updated itself. The current way is better but I'm not a fan of putting a bunch of lazy loading in such a hot function. The executionAsyncResource function will be called very frequently so it's best to pull as much of that cost as possible out of band from that call. That's a big part of why the resource_symbol property used to be computed ahead of time and only accessed later.

I'm still feeling like reverting the previous change is the better solution. It kind of sucks having two similar purposed properties, but much better for performance than trying to compute that every time it's accessed, which will happen more or at least as much and requires more logic to map back to what it was than just the original storing the resource in a symbol property.

@RaisinTen RaisinTen force-pushed the use-correct-resource-for-asynclocalstorage branch from 323f8a7 to 2615107 Compare November 13, 2021 07:58
@RaisinTen RaisinTen changed the title async_hooks: use correct resource for AsyncLocalStorage async_hooks: unmerge resource_symbol from owner_symbol Nov 13, 2021
@RaisinTen
Copy link
Contributor Author

@Qard

Yep, I had missed those changes when I wrote that comment as I had left the tab open for awhile to review before commenting and it had not updated itself. The current way is better but I'm not a fan of putting a bunch of lazy loading in such a hot function. The executionAsyncResource function will be called very frequently so it's best to pull as much of that cost as possible out of band from that call. That's a big part of why the resource_symbol property used to be computed ahead of time and only accessed later.

I'm still feeling like reverting the previous change is the better solution. It kind of sucks having two similar purposed properties, but much better for performance than trying to compute that every time it's accessed, which will happen more or at least as much and requires more logic to map back to what it was than just the original storing the resource in a symbol property.

I ran some benchmarks locally and indeed there was a 23% slow down for benchmark/async_hooks/async-local-storage-run.js because of the lazy loads, so I unmerged the symbols, PTAL

@nodejs-github-bot

This comment has been minimized.

orgads added a commit to orgads/docker-node that referenced this pull request Nov 18, 2021
Source: nodejs/node#40741

Pushed as orgads/node:16.13.0-asyncfix-alpine
RaisinTen added a commit that referenced this pull request Nov 18, 2021
Fixes: #40693

Signed-off-by: Darshan Sen <[email protected]>

PR-URL: #40741
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
RaisinTen added a commit that referenced this pull request Nov 18, 2021
This reverts commit 937bbc5.

PR-URL: #40741
Fixes: #40693
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
RaisinTen added a commit that referenced this pull request Nov 18, 2021
This reverts commit 7ca2f13.

PR-URL: #40741
Fixes: #40693
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@RaisinTen
Copy link
Contributor Author

Landed in 1b8bfdd, ff39895 and 2b0087f

@RaisinTen RaisinTen closed this Nov 18, 2021
@RaisinTen RaisinTen deleted the use-correct-resource-for-asynclocalstorage branch November 18, 2021 13:16
@RaisinTen RaisinTen removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 18, 2021
@orgads
Copy link
Contributor

orgads commented Nov 18, 2021

Thank you very much. Will it be backported to v16 automatically?

@RaisinTen
Copy link
Contributor Author

@orgads it would normally be done by PRs like #40504. If it doesn't land cleanly, someone will add a backport-requested label to this PR and then someone would have to do it manually to resolve the git conflicts.

@orgads
Copy link
Contributor

orgads commented Nov 18, 2021

Thanks.

targos pushed a commit that referenced this pull request Nov 21, 2021
Fixes: #40693

Signed-off-by: Darshan Sen <[email protected]>

PR-URL: #40741
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
targos pushed a commit that referenced this pull request Nov 21, 2021
This reverts commit 937bbc5.

PR-URL: #40741
Fixes: #40693
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
targos pushed a commit that referenced this pull request Nov 21, 2021
This reverts commit 7ca2f13.

PR-URL: #40741
Fixes: #40693
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Nov 26, 2021
1 task
@willianpc
Copy link

Hello folks, is there a particular reason why this change was not back ported into v16.x yet?
Are there near future plans to do it? I see that v17 is updated, but nothing landed into v16.x, back ported or not.

Cheers,

  • Willian

@orgads
Copy link
Contributor

orgads commented Jan 10, 2022

When is the next 16 release planned? It's been a month since the last minor release. Previous releases were more frequent.

I couldn't find a release roadmap/schedule anywhere.

@richardlau
Copy link
Member

Currently planned for sometime in January nodejs/Release#658

danielleadams pushed a commit that referenced this pull request Jan 30, 2022
Fixes: #40693

Signed-off-by: Darshan Sen <[email protected]>

PR-URL: #40741
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
This reverts commit 937bbc5.

PR-URL: #40741
Fixes: #40693
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
This reverts commit 7ca2f13.

PR-URL: #40741
Fixes: #40693
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
Fixes: #40693

Signed-off-by: Darshan Sen <[email protected]>

PR-URL: #40741
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
This reverts commit 937bbc5.

PR-URL: #40741
Fixes: #40693
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
This reverts commit 7ca2f13.

PR-URL: #40741
Fixes: #40693
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
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. author ready PRs that have at least one approval, no pending requests for changes, and a CI started.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REG 16.6->16.7] TCP/TLS drops AsyncLocalStorage