Emit destroy hook when AsyncResource has fulfilled its purpose

[devoto13]

Fixes #85

[dougwilson]

Thank you, very interesting! Is it possible to do this without modifying where you did? You modified our inline function that is supposed to just backport AsyncResource.bind from Node.js core, ideally so we can switch to that when poasible. We do not want our impl to diverge from AsyncResource.bind.

Also, please add a test to our test suite for this, that will fail without this change and paas with the change ❤️

[dougwilson]

See comment

[devoto13]

Thanks for the review @dougwilson!

I've looked more into it and the bind function is the same as the polyfill. Which made me wonder how does that work and I learned that Node emits the destroy hook when the corresponding object is garbage-collected.

So looks like it is more of an integration issue than a bug in raw-body. GC is not called in time and Jest thinks that async resource is still active. May still be good to emit destroy manually after the callback, but then raw-body won't be able to migrate to the AsyncResource.bind because we'll need to retain a reference to the instance of the AsyncResource. Would you still be willing to accept a PR? I'll then proceed with adding the test.

The workaround is to manually call GC (and it solves the issue), but that requires passing --expose-gc flag to Node which is rather inconvenient.

[dougwilson]

That seems odd, then how would anyone use the AsyncResource.bind in Node.js? We definitely do not want to diverge from Node.js core and be unable to use AsyncResource.bind here for compatible Node.js versions. Maybe this is worth reaching out to Node.js to understand how one would use it, or maybe reach out to Jest if this is a race condition that needs to be accounted for there? It seems like we should due some extra due diligence around this in light of thay information before determining the exact thing to land here just yet.

[devoto13]

Well, as I said, it's not really a bug in either of the places, it's the interaction which is the problem.

• From Node perspective, destroy is emitted when the AsyncResource is garbage collected, so everything works correctly from that perspective.
• From raw-body perspective, it uses the Node API as intended, so nothing is really broken.
• From Jest perspective, well, it listens to init and destroy events and expects AsyncResources to fire destroy once they are no longer needed. It's just the timing of "no longer needed" is too late when it comes to raw-body library because it depends on when GC runs.

I don't think Node can "fix" bind function in any way, since it has no idea whether that function will be called more or not, so relying on the GC is reasonable. And Node does allow users of the AsyncResource to emit the event early if they no longer need it (that's what this PR does).

I don't think Jest can change their logic to wait for/trigger GC, because GC is not available by default and forcing all consumers to pass --expose-gc flag is not going to happen.

raw-body however can address the issue, because raw-body does know that the callback will not be used again and it can emit the destroy without waiting for GC which will run at unknown point in the future. Hence my proposal to address the issue here.

Based on the above, I don't think escalation will bring us much.

[devoto13]

To me it really boils down to whether you're as a library author are willing to bring in extra complexity to make the library play nice with Jest out of the box or leave that burden to the Jest user to figure it out (by e.g. calling GC manually). IMO it would be nice to make the integration work out of the box, but I can totally understand if you don't want to have this extra complexity and then we can close this PR and I can workaround it on the consumer side.

[dougwilson]

The project intends to handle wrapping event emitters for async hooks following the Node.js documentation. https://nodejs.org/api/async_context.html#integrating-asyncresource-with-eventemitter