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

wgo doesn't shutdown when using templ --cmd #8

Closed
guilhas07 opened this issue Apr 14, 2024 · 10 comments
Closed

wgo doesn't shutdown when using templ --cmd #8

guilhas07 opened this issue Apr 14, 2024 · 10 comments

Comments

@guilhas07
Copy link
Contributor

guilhas07 commented Apr 14, 2024

I'm trying to use templ to restart on templ file changes with the additional --cmd flag to run wgo in order to watch go files:

  • templ generate --watch --proxy="http://localhost:8000" --cmd="wgo run main.go"

However, when templ restarts, the wgo cmd seems to not shutdown correctly, producing the error: listen tcp :8000: bind: address already in use as opposed to the same example using simply go:

  • Using go run main.go a new executable is generated:

image

  • Using wgo run main.go seems to keep the previous executable despite the wgo process being dead:

image

OS: WSL2 Ubuntu 22.04

I don't know if this behaviour is intended or not. Let me know if you need more info!

@bokwoon95
Copy link
Owner

Do you have some sample code I could try with? As well as which files to change to trigger the error, etc

@bokwoon95
Copy link
Owner

I suspect I know the reason -- templ is using SIGKILL to kill its child processes, which doesn't give them a chance to clean up and causes to wgo die immediately, leaving its own children behind. But I need to replicate the problem first and then test if changing from SIGKILL to SIGTERM works.

Ordinarily this won't be a problem, because templ sets a new process group ID for its descendants and kills them by their process group ID. But wgo pulls the same trick and also assigns its descendants a new process group ID. This means that wgo and wgo's descendants share different process group IDs, and templ killing its children by process group ID means only wgo gets killed, not wgo's descendants (they don't have the same process group ID). The correct way would be to kill wgo with SIGTERM so that wgo can intercept the SIGTERM and kill its own children (SIGKILL is too aggressive and can't be intercepted).

@guilhas07
Copy link
Contributor Author

Do you have some sample code I could try with? As well as which files to change to trigger the error, etc

main.go

package main

import (
	"log"
	"net/http"
)

func main() {
	log.Print("Listening on port 8080")
	log.Fatal(http.ListenAndServe(":8080", nil))
}

file.templ

package main

templ test(name1 string) {
	<div><p>{ name1 }</p></div>
}

command: templ generate --watch --cmd "wgo run main.go"

Steps to reproduce:

  • Create main.go and file.templ files.
  • Run the command above
  • Make a change to the file.templ file e.g., change the variable name

@guilhas07
Copy link
Contributor Author

The correct way would be to kill wgo with SIGTERM so that wgo can intercept the SIGTERM and kill its own children (SIGKILL is too aggressive and can't be intercepted).

Hmm, interesting. So what is your suspicion for the correct behaviour when using the go command instead?

@bokwoon95
Copy link
Owner

bokwoon95 commented Apr 16, 2024

Hmm, interesting. So what is your suspicion for the correct behaviour when using the go command instead?

Because the go command doesn't allocate a new process group ID for its children (so its children inherit the go command's process group ID). Killing that process group ID kills everything cleanly. In contrast, wgo does allocate a new process group ID for its children by nature of it being a supervisor process i.e. it has to be able to kill other process groups without being killed itself, so it possesses a separate process group ID from its children. Killing wgo's process group ID will not kill its children.

Here is an example program that spawns a child process.

main.go

package main

import (
    "os/exec"
    "log"
)

func main() {
    cmd := exec.Command("/bin/sh", "-c", "sleep 3600")
    log.Println("sleeping...")
    err := cmd.Run()
    if err != nil {
        log.Fatal(err)
    }
}

go run with SIGKILL (clean exit):

$ go run main.go
2024/04/16 19:50:39 sleeping...
# Press CTRL+Z to put the process into the background.
# Check running processes.
$ ps -j
  PID  PGID   SID TTY          TIME CMD
  136   136   136 pts/0    00:00:00 bash
 3136  3136   136 pts/0    00:00:00 go # <-- PGID of go command is 3136
 3204  3136   136 pts/0    00:00:00 main
 3209  3136   136 pts/0    00:00:00 sh
 3210  3136   136 pts/0    00:00:00 sleep
 3211  3211   136 pts/0    00:00:00 ps
# sigkill PGID 3136
$ kill -s SIGKILL -- -3136
# All processes with PGID 3136 were killed.
$ ps -j
  PID  PGID   SID TTY          TIME CMD
  136   136   136 pts/0    00:00:00 bash
 3212  3212   136 pts/0    00:00:00 ps

wgo run with SIGKILL (unclean exit):

$ wgo run main.go
2024/04/16 19:54:22 sleeping...
# Press CTRL+Z to put the process into the background.
# Check running processes.
$ ps -j
  PID  PGID   SID TTY          TIME CMD
  136   136   136 pts/0    00:00:00 bash
 3213  3213   136 pts/0    00:00:00 wgo # <-- PGID of wgo command is 3213
 3288  3288   136 pts/0    00:00:00 wgo_20240416195
 3293  3288   136 pts/0    00:00:00 sh
 3294  3288   136 pts/0    00:00:00 sleep
 3295  3295   136 pts/0    00:00:00 ps
# sigkill PGID 3213
$ kill -s SIGKILL -- -3213
# Only wgo was killed (PGID 3213), wgo's child processes (PGID 3288) are still hanging around.
$ ps -j
  PID  PGID   SID TTY          TIME CMD
  136   136   136 pts/0    00:00:00 bash
 3288  3288   136 pts/0    00:00:00 wgo_20240416195
 3293  3288   136 pts/0    00:00:00 sh
 3294  3288   136 pts/0    00:00:00 sleep
 3296  3296   136 pts/0    00:00:00 ps

wgo run with SIGTERM (clean exit):

$ wgo run main.go
2024/04/16 19:56:13 sleeping...
# Press CTRL+Z to put the process into the background.
# Check running processes.
$ ps -j
  PID  PGID   SID TTY          TIME CMD
  136   136   136 pts/0    00:00:00 bash
 3299  3299   136 pts/0    00:00:00 wgo # <-- PGID of wgo is 3299
 3365  3365   136 pts/0    00:00:00 wgo_20240416195
 3372  3365   136 pts/0    00:00:00 sh
 3373  3365   136 pts/0    00:00:00 sleep
 3374  3374   136 pts/0    00:00:00 ps
# sigterm PGID 3299
$ kill -s SIGTERM -- -3299
# wgo was killed (PGID 3299), but it intercepted SIGTERM and was able to kill its
# child processes (PGID 3365) before exiting.
$ ps -j
 PID  PGID   SID TTY          TIME CMD
 136   136   136 pts/0    00:00:00 bash
3375  3375   136 pts/0    00:00:00 ps

I've checked, changing all instances of SIGKILL to SIGTERM in templ/cmd/templ/generatecmd/run/run_unix.go does indeed solve the problem.

@guilhas07
Copy link
Contributor Author

guilhas07 commented Apr 16, 2024

Thank you for the thorough explanation!
So I supposed the way forward is to make a PR on the temple repo?

@bokwoon95
Copy link
Owner

Yup, you can do that

@tommyo
Copy link

tommyo commented Apr 25, 2024

You're using 2 different tools to do the same thing. Don't use --watch on templ generate to run wgo, use wgo to run templ generate

https://templ.guide/commands-and-tools/hot-reload#alternative-1-wgo

@guilhas07
Copy link
Contributor Author

You're using 2 different tools to do the same thing. Don't use --watch on templ generate to run wgo, use wgo to run templ generate

https://templ.guide/commands-and-tools/hot-reload#alternative-1-wgo

That doesn't do the same thing though. Correct me if I'm wrong but I believe that way templ generate would run every time a go file changed, and not only a templ file. And it doesn't have the proxy benefit nor the txt fast recompilation benefit of the templ --watch command.

This working setup allows me to get all the benefits above:

  • templ generate --watch --proxy="http://localhost:8000"& wgo run .

@guilhas07
Copy link
Contributor Author

Closed due to a-h/templ#687 being merged!

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

No branches or pull requests

3 participants