Conversation
yevgenypats
left a comment
There was a problem hiding this comment.
Really good catch. found one bug
disq
left a comment
There was a problem hiding this comment.
We need the same logic in the destinationclient as well.
…-sdk into fix-log-streaming-race
disq
left a comment
There was a problem hiding this comment.
still need the same fix in the destination client.
yevgenypats
left a comment
There was a problem hiding this comment.
I think it needs a few more tweaks. still race conditions here.
yevgenypats
left a comment
There was a problem hiding this comment.
Commented on the kill side (prob I should just use the comments - sorry for the re-review)
clients/source.go
Outdated
| } | ||
| if c.cmd != nil && c.cmd.Process != nil { | ||
| if err := c.cmd.Process.Kill(); err != nil { | ||
| // if we fail to kill the process, we also won't wait for logs to finish streaming |
There was a problem hiding this comment.
I don't think this comment is correct or at least we need to see what are the error message from kill.
This is the snippet from signal, it really can only fail if it already closed. you can always kill a child process. so we do want to wait for the logs if it is already killed.
func (p *Process) signal(sig Signal) error {
if p.Pid == -1 {
return errors.New("os: process already released")
}
if p.Pid == 0 {
return errors.New("os: process not initialized")
}
p.sigMu.RLock()
defer p.sigMu.RUnlock()
if p.done() {
return ErrProcessDone
}
s, ok := sig.(syscall.Signal)
if !ok {
return errors.New("os: unsupported signal type")
}
if e := syscall.Kill(p.Pid, s); e != nil {
if e == syscall.ESRCH {
return ErrProcessDone
}
return e
}
return nil
}
🤖 I have created a release *beep* *boop* --- ## [0.12.4](v0.12.3...v0.12.4) (2022-10-04) ### Bug Fixes * Improve download message ([#240](#240)) ([7929bbb](7929bbb)) * Race condition in log streaming ([#242](#242)) ([3c8242a](3c8242a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This adds a sync.WaitGroup to ensure that we wait for all logs to finish streaming before returning from Terminate. Otherwise, the log file might get closed before we get to write the final lines.