Skip to content

fix: stop existing child process before spawning a new one Closes #9#28

Open
khushi-saini-23 wants to merge 2 commits into
thunderavi:mainfrom
khushi-saini-23:fix/dev-watcher-restart-cleanup
Open

fix: stop existing child process before spawning a new one Closes #9#28
khushi-saini-23 wants to merge 2 commits into
thunderavi:mainfrom
khushi-saini-23:fix/dev-watcher-restart-cleanup

Conversation

@khushi-saini-23
Copy link
Copy Markdown

Fixed the development watcher cleanup logic in cli/index.js by tracking the active child process and gracefully killing it using spawn before triggering a new run.

Closes #9

Copy link
Copy Markdown
Owner

@thunderavi thunderavi left a comment

Choose a reason for hiding this comment

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

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 = null

This 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.

@khushi-saini-23
Copy link
Copy Markdown
Author

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!

@thunderavi
Copy link
Copy Markdown
Owner

Hi @khushi-saini-23, thanks for updating the PR. The restart race issue is fixed now, and CI is passing.

I tested the rapid restart behavior locally with a mocked child process, and the previous active process is now killed correctly.

Before merge, please fix two remaining things:

  1. runEasyJS() is used by both dev and start. After replacing exec with spawn, the CLI no longer exits with a failure code when the child process exits with an error. Please preserve the previous behavior for easyjs start, so a failed server process causes the CLI to exit with a non-zero code.

  2. Please remove the unrelated package-lock.json changes from this PR. This issue should only need the watcher/CLI logic.

Also, please add a small test for rapid restarts if possible, as discussed earlier.

After these changes, I can re-review and approve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

Fix dev watcher so it restarts the existing server instead of spawning duplicates

2 participants