Skip to content

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

Merged
AriPerkkio merged 1 commit intotinylibs:mainfrom
AriPerkkio:fix/clean-up-async-resources
Apr 21, 2024
Merged

fix: pool.destroy to clean up async resources#86
AriPerkkio merged 1 commit intotinylibs:mainfrom
AriPerkkio:fix/clean-up-async-resources

Conversation

@AriPerkkio
Copy link
Copy Markdown
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
Copy Markdown
Member Author

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

@AriPerkkio AriPerkkio requested a review from Aslemammad April 21, 2024 08:34
@AriPerkkio
Copy link
Copy Markdown
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
Copy Markdown
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
@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.

2 participants