fix: stop existing child process before spawning a new one Closes #9#28
fix: stop existing child process before spawning a new one Closes #9#28khushi-saini-23 wants to merge 2 commits into
Conversation
thunderavi
left a comment
There was a problem hiding this comment.
Hi @khushi-saini-23, thanks for the PR. CI is passing and the approach is in the right direction, but there is still one restart race issue before we can merge.
In runEasyJS(), the old child process close handler always does:
this.activeChildProcess = nullThis can clear the reference to the newly spawned process if the old process closes after the new one has already started. Then the next restart will not kill the active server, so duplicate processes can still happen.
Please update the logic so the close handler only clears the reference if it belongs to the same child process, for example:
const child = spawn(...)
this.activeChildProcess = child
child.on('close', () => {
if (this.activeChildProcess === child) {
this.activeChildProcess = null
}
})Also, if possible, add a test or small coverage for rapid restarts to prove the previous child is killed before another one is started.
After this fix, I will review again.
|
Hi @thunderavi, great catch! I completely agree with the race condition issue. I will update the close handler logic to verify the child process instance and add a test case for rapid restarts right away. Thanks! |
Fixed the development watcher cleanup logic in
cli/index.jsby tracking the active child process and gracefully killing it usingspawnbefore triggering a new run.Closes #9