async_hooks: unmerge resource_symbol from owner_symbol

[RaisinTen]

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

Fixes: #40693

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

[targos]

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

[RaisinTen]

cc @nodejs/async_hooks

[vdeturckheim]

@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]

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

[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. 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 regularowner_symbol stuff just grabs the direct owner while executionAsyncResource(...) keeps following the links until it finds something without an owner?

[RaisinTen]

@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 regularowner_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?

[addaleax]

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]

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]

@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]

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]

@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

[RaisinTen]

Landed in 1b8bfdd, ff39895 and 2b0087f

[orgads]

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

[RaisinTen]

@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]

Thanks.

[willianpc]

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]

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]

Currently planned for sometime in January nodejs/Release#658