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

fix: send SIGTERM signal to --cmd instead of SIGKILL #687

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

guilhas07
Copy link

Problem

When trying to use the command templ generate --watch --proxy="http://localhost:8000" --cmd="wgo run main.go" I noticed that wgo didn't close correctly. After discussing on the wgo repo ( see here ) the conclusion was that this was due to templ sending a SIGKILL signal not allowing for wgo to close correctly.

Solution

To fix this I changed all SIGKILL signals to SIGTERM.

Caveats:

  • I don't know if this is a problem too on windows
  • I suppose this doesn't break any behaviour, but let me know if it does.

Let me know if there is something I need to change!

@a-h
Copy link
Owner

a-h commented Apr 17, 2024

Thanks, I'll have a read through of the other repo. Sigkill kills child processes, where sigterm doesn't, so there's a good reason to use kill - for example, if you do go run, it does a build, then runs the executable as a child process, so sigterm wouldn't necessarily kill that process.

@guilhas07
Copy link
Author

guilhas07 commented Apr 17, 2024

Thanks, I'll have a read through of the other repo. Sigkill kills child processes, where sigterm doesn't, so there's a good reason to use kill - for example, if you do go run, it does a build, then runs the executable as a child process, so sigterm wouldn't necessarily kill that process.

Using go run seems to be working as expected with this version too. Using this example bokwoon95/wgo#8 (comment) from the issue above.

Isn't the propagation of the signal dependent on the process group id, so even if we use SIGKILL, we need to send to the group id so that the children will terminate https://stackoverflow.com/questions/21869242/killing-parent-process-along-with-child-process-using-sigkill . So by using SIGTERM it would still be fine if we send to the process group id, right?

@a-h
Copy link
Owner

a-h commented May 12, 2024

Reading through Air's code, it appears that it can send a SIGTERM to initiate graceful shutdown, prior to attempting to forcefully kill the process after a timeout: https://github.com/cosmtrek/air/blob/3f19370fe5e8d8fe2ddd927de54b1aede379f2b7/runner/util_unix.go#L13

I think that's what templ needs to do in order to allow wgo to shutdown any processes it's started gracefully.

@guilhas07
Copy link
Author

guilhas07 commented May 12, 2024

Reading through Air's code, it appears that it can send a SIGTERM to initiate graceful shutdown, prior to attempting to forcefully kill the process after a timeout: https://github.com/cosmtrek/air/blob/3f19370fe5e8d8fe2ddd927de54b1aede379f2b7/runner/util_unix.go#L13

I think that's what templ needs to do in order to allow wgo to shutdown any processes it's started gracefully.

I believe the current way in this PR should be correct because we should not assume a timeout for how long the user command takes in order to execute its clean-up logic. ref: https://stackoverflow.com/a/690631/12458551 .

Edit: Maybe in this context, it would be actually useful to have a timeout anyways in favour of a better user experience in variable scenarios.

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.

None yet

2 participants