Skip to content

fix(wasm): shutdown async runtime and thread workers after finishing#9895

Merged
h-a-n-a merged 6 commits intoweb-infra-dev:mainfrom
CPunisher:04-03-fix/wasi-execution-shutdown
Apr 15, 2025
Merged

fix(wasm): shutdown async runtime and thread workers after finishing#9895
h-a-n-a merged 6 commits intoweb-infra-dev:mainfrom
CPunisher:04-03-fix/wasi-execution-shutdown

Conversation

@CPunisher
Copy link
Copy Markdown
Contributor

@CPunisher CPunisher commented Apr 3, 2025

Summary

The reason I make this pr separately is that I'm not sure if my solution is best. So discussions are welcome.

Rspack wasi will get stuck after finishing, because:

  1. We should manually shutdown napi async runtime.
  2. Rayon thread pool doesn't exit. Normally rayon threads run and steal works until the main thread exits. But wasi simulates threads with nodejs workers. I find the main thread in nodejs doesn't exit if nodejs worker get stuck. So we should also shutdown all the workers manually.

Temporarily I stop napi generating js glue code and export the function to shutdown workers. And I'm not sure the replace where I put shutdown code is correct.

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

@github-actions github-actions bot added the release: bug fix release: bug related release(mr only) label Apr 3, 2025
@netlify
Copy link
Copy Markdown

netlify bot commented Apr 3, 2025

Deploy Preview for rspack canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 8fc3b7d
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/67f9d4694495670008a95e4d

@CPunisher CPunisher requested a review from h-a-n-a April 3, 2025 08:29
@h-a-n-a
Copy link
Copy Markdown
Contributor

h-a-n-a commented Apr 3, 2025

Would you please elaborate on "I find the main thread in nodejs doesn't exit if nodejs worker get stuck."? Is this problem exist when nodejs workers encounter some error or exist on every executions?

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Apr 3, 2025

CodSpeed Performance Report

Merging #9895 will not alter performance

Comparing CPunisher:04-03-fix/wasi-execution-shutdown (8fc3b7d) with main (dfc27d8)

Summary

✅ 11 untouched benchmarks

@CPunisher
Copy link
Copy Markdown
Contributor Author

CPunisher commented Apr 3, 2025

Would you please elaborate on "I find the main thread in nodejs doesn't exit if nodejs worker get stuck."? Is this problem exist when nodejs workers encounter some error or exist on every executions?

Here is an example. When you run node main.js, you will get stuck.

// main.js
const { Worker } = require('worker_threads');
const worker = new Worker('./worker.js')
console.log("main end")

// worker.js
console.log("worker start")
while (true) {}
console.log("worker end")

Now let me clarify the problems with rayon:

  1. Rspack wasi (powered by emnapi) create threads by creating node workers
  2. Although rayon falls back to sequential iteration in wasm, I find it still create its worker threads which lives in the whole lifetime of program. In normal native program, it's ok because the main thread doesn't wait for child threads without join.
  3. Rolldown avoids using rayon by wasm shims: https://github.com/rolldown/rolldown/blob/main/crates/rolldown_utils/src/rayon.rs. It's very hard to apply it in rspack because some rayon iter api is not consistent with std iter api such as fold. We may need to do more to improve the wasm shims or write lots of #[cfg].

@h-a-n-a
Copy link
Copy Markdown
Contributor

h-a-n-a commented Apr 3, 2025

Although rayon falls back to sequential iteration in wasm, I find it still create its worker threads which lives in the whole lifetime of program.

That's weird. I haven't really dug into the implementation in WASI node. But I thought rayon tasks would be sent to worker_threads for execution instead of being executed in sequence.

In normal native program, it's ok because the main thread doesn't wait for child threads without join.

Is it possible to unref() all Workers? This should make the worker threads exit if main thread finishes its execution?

@CPunisher
Copy link
Copy Markdown
Contributor Author

That's weird. I haven't really dug into the implementation in WASI node. But I thought rayon tasks would be sent to worker_threads for execution instead of being executed in sequence.

You could be right! Actually I know this from https://github.com/rayon-rs/rayon?tab=readme-ov-file#usage-with-webassembly, but I think it's very valuable to optimize in the future.

Is it possible to unref() all Workers? This should make the worker threads exit if main thread finishes its execution?

Thanks! I didn't know this api before. I quickly tried but didn't solve the problem. I will do more research about it.

@CPunisher CPunisher changed the title fix(wasm): shutdown async runtime and thread workers after finishing [WIP] fix(wasm): shutdown async runtime and thread workers after finishing Apr 4, 2025
@github-actions github-actions bot removed the release: bug fix release: bug related release(mr only) label Apr 4, 2025
@CPunisher CPunisher mentioned this pull request Apr 4, 2025
8 tasks
@CPunisher CPunisher force-pushed the 04-03-fix/wasi-execution-shutdown branch from 394d2ee to d5d12b9 Compare April 11, 2025 15:31
@CPunisher
Copy link
Copy Markdown
Contributor Author

It seems that worker.on('message', ...) will call worker.ref() again, see: nodejs/node#53036. A hacky way is adding worker.ref = () => {} to prevent workers being refed.

@CPunisher CPunisher changed the title [WIP] fix(wasm): shutdown async runtime and thread workers after finishing fix(wasm): shutdown async runtime and thread workers after finishing Apr 12, 2025
@github-actions github-actions bot added the release: bug fix release: bug related release(mr only) label Apr 12, 2025
Copy link
Copy Markdown
Contributor

@h-a-n-a h-a-n-a left a comment

Choose a reason for hiding this comment

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

Thanks

@h-a-n-a h-a-n-a merged commit 73b9679 into web-infra-dev:main Apr 15, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release: bug fix release: bug related release(mr only)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants