-
Notifications
You must be signed in to change notification settings - Fork 334
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
chore: Gracefully shutdown the embedder #9693
Conversation
throw err | ||
}, | ||
return: async () => { | ||
await Promise.allSettled(iterators.map((iterator) => iterator.return?.())) |
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.
This refactoring was done so we can define this return function. This is necessary so all streams are notified of the return. The async generator function we had previously would hang in await capability.promise
and handle the return only afterwards. If all embedders are idle, the promise would never resolve.
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.
interesting! really good find having to define our own return
here
@@ -6,5 +6,5 @@ export const logMemoryUse = () => { | |||
const {rss} = memoryUsage | |||
const usedMB = Math.floor(rss / MB) | |||
console.log('Memory use:', usedMB, 'MB') | |||
}, 10000) | |||
}, 10000).unref() |
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.
This was an attempt to regularly end the event loop. It will continue if there are still timers running unless these are unref
ed. This did not allow proper shutdown as there are many things in dependencies still running, but it's good practice nonetheless if we're not keeping a reference to the timer to allow cleanup.
Description
Fixes #9693
End all streams on receiving a kill signal to properly shutdown the embedder. Ending just the merged stream does not work in the current situation as it will wait indefinitely if all the workers are idle.
Demo
Nothing to see here.
Testing scenarios
SIGINT
via Ctrl+CFinal checklist