Skip to content
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

Prevent project shutdown on watch mode #47

Merged
merged 1 commit into from
Jul 7, 2022

Conversation

elyse0
Copy link
Contributor

@elyse0 elyse0 commented Jul 6, 2022

Hi, I've just discovered this package and I was surprised on how fast it was 🚀

Unfortunately I tried using it on watch mode, but the program kept ending on any file change (apparently someone else has already noticed the same issue #45 )

So... this is my shot into fixing it 🐞

Issue

In order to react to changes in our files, the watcher triggers a change event, that in turn triggers a project reload.

This project reload forces a supervisor restart, that in the code is implemented by killing the current process and creating a new one.

wds/src/Supervisor.ts

Lines 32 to 46 in f89b208

restart() {
if (this.process) {
this.process.kill("SIGKILL");
}
this.process = spawn("node", this.argv, {
cwd: process.cwd(),
env: {
...process.env,
WDS_SOCKET_PATH: this.socketPath,
WDS_EXTENSIONS: this.project.config.extensions.join(","),
},
stdio: [null, "inherit", "inherit", "ipc"],
});

This process exit is caught in the supervisor exit event, but it's only prevented for the supervise mode case.

wds/src/index.ts

Lines 172 to 177 in f89b208

project.supervisor.process.on("exit", (code) => {
log.debug(`child process exited with code ${code}, ${options.supervise ? "not exiting because supervise mode" : "exiting..."}`);
if (!options.supervise) {
project.shutdown(code ?? 1);
}
});

Possible solution

My proposed solution is to add the missing reload on changes mode case, preventing to kill our 'project' on any file change.

Images

Running original file (watch mode is enabled by default)
image

Modifying and saving changes to file
image

Previously the program would just end, like in the logs of #45

@airhorns
Copy link
Contributor

airhorns commented Jul 6, 2022

Ah, thanks for the PR! I think I've always just run with wds --watch --supervise to turn on both bits of behaviour! I agree though that it isn't very useful to turn on --watch without also turning on the supervise behaviour -- you told wds you wanted it to watch and that you're gonna make changes, seems silly it would just quit! What do you think about making the --watch flag imply the --supervise flag, documenting it like that and fixing the issue at a higher level when doing argparsing? Also curious what @gmalette thinks

@gmalette
Copy link
Collaborator

gmalette commented Jul 6, 2022

I've so far considered --watch to be fs-related, and --supervise to be essentially keep-alive, but that's perhaps the wrong way to think about it.

Given the role of wds, I can't think of a reason you'd want --supervise without --watch. Why keep the process alive, but not reload it with the changed code, except if it exited by itself and/or crashed?

I have used --watch without supervise to let the process die, when booting the process caused a significant amount of logs, making it hard to find the breakage.

Unless there's compelling arguments to keep --supervise, I think I'd be in favour of:

  1. Getting rid of --supervise
  2. --watch by default also keeps the project alive if the child process exits
  3. Add a flag to --watch that allows the process to die

Given that my usage of --watch without --supervise may be a special case, I'd also be open to deferring (3).

@elyse0
Copy link
Contributor Author

elyse0 commented Jul 6, 2022

Okay, I've removed supervise mode

  1. Add a flag to --watch that allows the process to die

I think--no-watch already does this, is that what you meant?

image

Copy link
Contributor

@airhorns airhorns left a comment

Choose a reason for hiding this comment

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

LGTM after one comment! Thanks!

src/index.ts Show resolved Hide resolved
There's no point in keeping the process alive on 'watch' mode, but
being unable to reload on any file changes. Before this fix, we would
need to use 'watch' and 'supervise' modes together to achieve this.

This fix makes 'watch' mode behave like 'supervise' mode, but getting
rid of the latter.
@gmalette
Copy link
Collaborator

gmalette commented Jul 7, 2022

I think--no-watch already does this, is that what you meant?

There's a subtle difference for long-running processes that don't automatically end. With only --watch changing its files will cause it to be reloaded. However, if it crashes (or exits normally for that matter), previously to this PR it would not have been restarted.

@airhorns airhorns merged commit d9a2dfa into gadget-inc:main Jul 7, 2022
@gmalette
Copy link
Collaborator

gmalette commented Jul 7, 2022

@airhorns can you publish a new version pls?

@elyse0
Copy link
Contributor Author

elyse0 commented Jul 7, 2022

There's a subtle difference for long-running processes that don't automatically end. With only --watch changing its files will cause it to be reloaded. However, if it crashes (or exits normally for that matter), previously to this PR it would not have been restarted.

Got it. I also noticed the tests keep running infinitely, it's probably because, as you say, now the process doesn't end

https://github.com/gadget-inc/wds/runs/7234432137

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants