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
Node: Add new worker.on('subprocessclose') to fix Node tests #1307
Conversation
Fixes #1306 ### Details - Problem only happened when running a test file separately (it worked by accident when running all together as usual). - Problem was that the `ChildProcess` object in `Worker` (the `this.#child` member) remains open for a bit until it emits 'close' event, which is emitted after 'exit' event or after 'error' event. - Here we have added a new 'subprocessclose' event in `Worker` which is emitted when the worker subprocess has closed. It may be (and it will be) emitted AFTER calling `worker.close()` but not immediately after it. - This event is basically useful for scenarios like testing where we want to verify that nothing remains running/open when a test is finished. - Also added a `worker.subprocessClosed` getter. - Removed an ugly `setTimeout` in `Channel::close()` method. If the app wants to get the very latest notifications/logs from worker when closing it, use the above event to wait for final closure. ### Bonus Tracks - Be consistent in command line arguments and always use `--option-foo bar` instead of `--option-foo=bar`.
@@ -138,7 +144,7 @@ test('dataProducer.send() succeeds', async () => { | |||
); | |||
}); | |||
|
|||
sendNextMessage(); | |||
await sendNextMessage(); |
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.
I have some hope that this will fix this CI random failure: https://github.com/versatica/mediasoup/actions/runs/7448459449/job/20262892577?pr=1305
ctx.worker?.close(); | ||
|
||
if (ctx.worker?.subprocessClosed === false) { |
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.
We check === false
because if the worker failed to be created in beforeEach
hook, then ctx.worker
won't exist here so ctx.worker?.subprocessClosed
will return undefined
. In that case we do NOT want to await for any event.
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.
Just added a cosmetic/style comment.
Fixes #1306
Details
ChildProcess
object inWorker
(thethis.#child
member) remains open for a bit until it emits 'close' event, which is emitted after 'exit' event or after 'error' event.worker.close()
method is sync and we do not care of what is happening in the subprocess after calling it. And this is good. However it's bad for tests if we use--detectOpenHandles
in Jest command.Worker
which is emitted when the worker subprocess has closed. It may be (and it will be) emitted AFTER callingworker.close()
but not immediately after it.worker.subprocessClosed
getter.setTimeout
inChannel::close()
method. If the app wants to get the very latest notifications/logs from worker when closing it, use the above event to wait for final closure.Bonus Tracks
--option-foo bar
instead of--option-foo=bar
.tasks.py
andMakefile
more resistant to paths with spaces.