remove a race condition in sdk testutil server stop #10342
Conversation
|
|
dnephin
added
theme/testing
There was a problem hiding this comment.
Thank you for the PR and for fixing this data race! We've been tracking the work to enable the race detector in #8329, but we had not seen this race yet.
I've made one suggestion below, but otherwise I think this looks correct.
| s.cmd.Process.Signal(syscall.SIGABRT) | ||
| s.cmd.Wait() |
There was a problem hiding this comment.
I think we do still want to wait for the process to exit in this case, because there is a defer at the top of this function which is responsible for removing the temporary directory. We want to make sure the process has exited before we try and remove it to make sure the cleanup succeeds.
I think we can do that by blocking on a read from the waitDone channel instead of calling s.cmd.Wait()
<-waitDone
return ...Sending SIGABRT should ensure the process exits, so it should be safe to block here in the main goroutine. We might want to change this signal to SIGKILL at some point, if the process is not shutting down with SIGABRT, but I think we can wait to make that change.
There was a problem hiding this comment.
Hello @dnephin that totally makes sense. Can you check the last commit ?
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/383609. |
We have 2 threads calling
exec.Cmd.Waitwhich is not reentrant. It causes some test case failure at our side when running the test suite with the-raceflag.I've just removed the
cmd.Waitwhich looks useless.Related to #8329