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

fix: pool.destroy to clean up async resources #86

Merged
merged 1 commit into from Apr 21, 2024

Conversation

AriPerkkio
Copy link
Member

@AriPerkkio AriPerkkio commented Apr 16, 2024

This issue came up while looking into vitest-dev/vitest#5544.

Tinypool's async resources show up in Vitest's hanging-process reporter.

class EventEmitterReferencingAsyncResource extends AsyncResource {

 RUN  v1.5.0 /Users/x/vitest/test/cli/fixtures/project

 ✓ |space_1| base.test.ts  (1 test) 2ms

 Test Files  1 passed (1)
      Tests  1 passed (1)
   Start at  21:40:23
   Duration  197ms (transform 17ms, setup 0ms, collect 12ms, tests 2ms, environment 0ms, prepare 66ms)

There are 12 handle(s) keeping the process running

# Tinypool
node:internal/async_hooks:202                                                                             
node:internal/async_hooks:505                                                                             
file:///Users/x/vitest/node_modules/.pnpm/tinypool@0.8.3/node_modules/tinypool/dist/esm/index.js:37 
file:///Users/x/vitest/node_modules/.pnpm/tinypool@0.8.3/node_modules/tinypool/dist/esm/index.js:58 
file:///Users/x/vitest/node_modules/.pnpm/tinypool@0.8.3/node_modules/tinypool/dist/esm/index.js:945
file:///Users/x/vitest/packages/vitest/dist/vendor/cac.dwzv-pfu.js:8776                             
file:///Users/x/vitest/packages/vitest/dist/vendor/cac.dwzv-pfu.js:9425                             
file:///Users/x/vitest/packages/vitest/dist/vendor/cac.dwzv-pfu.js:9450                             

# FILEHANDLE
node:internal/async_hooks:202

... more FILEHANDLES here

# FILEHANDLE
node:internal/async_hooks:202
close timed out after 10000ms
Tests closed successfully but something prevents 2 Vite servers from exiting
You can try to identify the cause by enabling "hanging-process" reporter. See https://vitest.dev/config/#reporters

After this changes the resource is cleaned up during pool.destroy() and it doesn't show up in reports anymore:

 RUN  v1.5.0 /Users/x/vitest/test/cli/fixtures/project

 ✓ |space_1| base.test.ts  (1 test) 1ms

 Test Files  1 passed (1)
      Tests  1 passed (1)
   Start at  21:41:19
   Duration  188ms (transform 12ms, setup 0ms, collect 8ms, tests 1ms, environment 0ms, prepare 53ms)

There are 11 handle(s) keeping the process running

# FILEHANDLE
node:internal/async_hooks:202

... more FILEHANDLES here

# FILEHANDLE
node:internal/async_hooks:202
close timed out after 10000ms
Tests closed successfully but something prevents 2 Vite servers from exiting
You can try to identify the cause by enabling "hanging-process" reporter. See https://vitest.dev/config/#reporters

@AriPerkkio
Copy link
Member Author

Need to do some more manual testing with Vitest but otherwise this should be good.

@AriPerkkio
Copy link
Member Author

Tested with Vitest with all built-in pools and it works fine. I'm not that familiar with Node's async_hooks though - @Aslemammad any thoughts about this PR?

Copy link
Member

@Aslemammad Aslemammad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@AriPerkkio AriPerkkio merged commit 994802b into tinylibs:main Apr 21, 2024
6 checks passed
@AriPerkkio AriPerkkio deleted the fix/clean-up-async-resources branch April 21, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants