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.

Sign up for GitHub

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

Jump to bottom

Implement graceful shutdown on Mac #619

Merged
merged 8 commits into from
Jun 24, 2024
Merged

Implement graceful shutdown on Mac #619

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
aa4a5ac
hold cleanup until procKilledCh signal
sethbrasile Apr 30, 2024
d1a45b8
signal to procKilledChan in existing deferred func
sethbrasile Apr 30, 2024
240f4c1
without this can't exit when non zero exit code
sethbrasile Apr 30, 2024
1f78251
Revert "without this can't exit when non zero exit code"
sethbrasile Apr 30, 2024
d312f15
use cleanup
xiantang Jun 23, 2024
cf8e797
fix test case rerun
xiantang Jun 23, 2024
360714a
upgrade gomod version
xiantang Jun 23, 2024
849e029
remove unused `WaitGroup`
xiantang Jun 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions runner/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type Engine struct {
binStopCh chan bool
exitCh chan bool

procKillWg sync.WaitGroup
mu sync.RWMutex
watchers uint
fileChecksums *checksumMap
Expand Down Expand Up @@ -478,8 +479,7 @@ func (e *Engine) runPostCmd() error {
}

func (e *Engine) runBin() error {
killFunc := func(cmd *exec.Cmd, stdout io.ReadCloser, stderr io.ReadCloser, killCh chan struct{}, processExit chan struct{}, wg *sync.WaitGroup) {
defer wg.Done()
killFunc := func(cmd *exec.Cmd, stdout io.ReadCloser, stderr io.ReadCloser, killCh chan struct{}, processExit chan struct{}) {
select {
// listen to binStopCh
// cleanup() will close binStopCh when engine stop
Expand Down Expand Up @@ -518,13 +518,11 @@ func (e *Engine) runBin() error {

e.runnerLog("running...")
go func() {
wg := sync.WaitGroup{}

defer func() {
select {
case <-e.exitCh:
e.mainDebug("exit in runBin")
wg.Wait()
default:
}
}()
Expand All @@ -536,6 +534,7 @@ func (e *Engine) runBin() error {
case <-killCh:
return
default:
e.procKillWg.Add(1)
Copy link

Choose a reason for hiding this comment

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

What I don't understand is why the WaitGroup passed to killFunc on line 553 doesn't seem to be working properly, I'd expect that to have waited for the process to be killed.

Copy link
Collaborator Author

@xiantang xiantang Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

let me take a look

Copy link

@nakkamarra nakkamarra Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No that's expected of course, but the WaitGroup that function expects as an argument is defined on 522 and is only ever Waited on line 528, so perhaps waiting on that wasn't working as expected or the Wait wasn't being called every time. To me, at least, figuring that out seems like a better solution than adding yet another WaitGroup

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

when i remove the wg.Done(), the we still can exit. i think it's pointer issue. because we put the pointer into funtion

Copy link
Collaborator Author

@xiantang xiantang Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

oh i see https://github.com/cosmtrek/air/blob/360714a021b1b77e50a5656fefc4f8bb9312d328/runner/engine.go#L520-L522, bcs it's running inside the new goroutine, and We didn't use something like join to let main goroutine to wait son goroutine, so main goroutine don't even FXXK care about that sub goroutine.
i think we should remove this useless WaitGroup

command := strings.Join(append([]string{e.config.Build.Bin}, e.runArgs...), " ")
cmd, stdout, stderr, _ := e.startCmd(command)
processExit := make(chan struct{})
Expand All @@ -544,11 +543,10 @@ func (e *Engine) runBin() error {
e.proxy.Reload()
}

wg.Add(1)
e.withLock(func() {
close(e.binStopCh)
e.binStopCh = make(chan bool)
go killFunc(cmd, stdout, stderr, killCh, processExit, &wg)
go killFunc(cmd, stdout, stderr, killCh, processExit)
})

go func() {
Expand All @@ -570,6 +568,7 @@ func (e *Engine) runBin() error {
default:
e.runnerLog("Process Exit with Code: %v", state.ExitCode())
}
e.procKillWg.Done()

if !e.config.Build.Rerun {
return
Expand Down Expand Up @@ -621,7 +620,7 @@ func (e *Engine) cleanup() {
}

e.mainDebug("waiting for exit...")

e.procKillWg.Wait()
e.running = false
e.mainDebug("exited")
}
Expand Down
Loading