nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.43k stars 29.01k forks source link

How to wrap event emitters with async_hooks #46699

Open dougwilson opened 1 year ago

dougwilson commented 1 year ago

Affected URL(s)

No response

Description of the problem

Hello,

I have been following https://nodejs.org/api/async_context.html#integrating-asyncresource-with-eventemitter for wrapping EventEmitters in libraries so the async context is preserved. I recently got a bug report that apparently this does not play well with the Jest test library, as Jest is apparently listening to async resource ls being created and destroyed and will warn/error the users if an async resource is created in a test but not destroyed before the test is finished.

I was at first going to use the asyncResource property added by AsyncResource.bind to manually destroy it, but I see that property is deprecated in master branch now.

So I am trying to better understand if there is a buy or oversight somewhere. Like does the pattern at https://nodejs.org/api/async_context.html#integrating-asyncresource-with-eventemitter need to be updated, does the depreciation need to be rolled back, or is Jest doing something it shouldn't/needs to take that pattern in to account.

I hope the above makes sense.

marco-ippolito commented 1 year ago

I'm a bit confused, could you please provide an example? Jest is known for monkey patching so it's hard to tell whats going on

dougwilson commented 1 year ago

Hi @marco-ippolito apologies, as I didn't get an example from the reporter. I also opened an issue with Jest and learned from them it was likely an issue with Jest that was resolved in Jest 29. I tried putting together a minimal test case and the warning seemed to have been gone from Jest 29 just as the Jest project suggested.

So since the issue seems to have been resolved with Jest 29, perhaps there is nothing that needs to be done here.

Maybe @devoto13 can help out either confirming that the issue is fixed with Jest 29 or with a reproduction case we can whittle down to what is happening, exactly.

devoto13 commented 1 year ago

Hey, Yes, the issue is indeed fixed by Jest 29 by explicitly calling GC to make sure that destroy hooks are fired for AsyncResources implicitly created by the AsyncResouce.bind call. See https://github.com/facebook/jest/commit/1f728031c6bbf389f0a646640445130e9f4424a7. https://github.com/stream-utils/raw-body/issues/85 is my original issue for the context.