-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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: try fix node crash #335
Conversation
Workers that are not in use are in unref state and process can shutdown gracefully anyway
As there is no pending work process will shutdown gracefully anyway
✔️ Deploy Preview for vitest-dev ready! 🔨 Explore the source changes: e8759b8 🔍 Inspect the deploy log: https://app.netlify.com/sites/vitest-dev/deploys/61c9fa4008a82600075c92ea 😎 Browse the preview: https://deploy-preview-335--vitest-dev.netlify.app |
@@ -101,9 +101,6 @@ async function run(cliFilters: string[], options: UserConfig) { | |||
if (!ctx.config.watch) | |||
await ctx.close() | |||
} | |||
|
|||
if (!ctx.config.watch) | |||
process.exit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't have this, the process might be hanging due to unresolved side effects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Terminating the process causes the crash too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you have an issue with unresolved effects or this is just a precaution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NodeJs workers are really fragile. If there anything going on inside worker (open port, file handle), terminating it causes a crash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Demivan With your changes windows will run ok, testing last changes (3 consecutive nr ci
+ 3 sequences alternating pnpm run test:cli
and pnpm run test:all
)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be dangerous as a hanging process will eat up all your CI time 👀. At some point, there are some cases, but not very sure if it still applies since we migrate to workers (but there is still a non-worker mode). Could never be too prepared :P
Co-authored-by: Anthony Fu <[email protected]>
Fixes #319