Remove race around local-exec Wait #11554
Conversation
jbardin
force-pushed
the
jbardin/local-exec-context
branch
from
8bc9d48
to
ad86d63
Compare
jbardin
force-pushed
the
jbardin/local-exec-context
branch
11 times, most recently
from
95da56d
to
9c5b300
Compare
There was still a race around the local-exec Command, where we were calling Wait in 2 places which you can't do.
jbardin
force-pushed
the
jbardin/local-exec-context
branch
from
9c5b300
to
e0325d9
Compare
|
Tests were passing locally on macOS. The problem looks like a Go issue on Linux: golang/go#18874 |
|
It appears this is completely broken, and we can't Wait on canceled process on Linux right now. This was only working before by returning the wrong error and leaving the process running. |
jbardin
force-pushed
the
jbardin/local-exec-context
branch
from
d385c6b
to
9182433
Compare
|
|
||
| multiWriter := io.MultiWriter(output, pw) | ||
|
|
||
| // now we need an os.Pipe to pump the outout on Linux |
There was a problem hiding this comment.
I would add the link to the Go issue here so we know when we can remove it.
jbardin
force-pushed
the
jbardin/local-exec-context
branch
from
9182433
to
cb9b5c0
Compare
If the shell spawns a subprocess which doesn't close the output file descriptors, the exec.Cmd will block on Wait() (see golang.org/issue/18874). Use an os.Pipe to provide the command with a real file descriptor so the exec package doesn't need to do the copy manually. This in turn may block our own reading goroutine, but we can select on that and leave it for cleanup later.
jbardin
force-pushed
the
jbardin/local-exec-context
branch
from
cb9b5c0
to
ff2936b
Compare
|
Turns out it's not a Linux problem, it just showed up on Linux as Having the process output to an os.Pipe prevents the exec package from needing to start goroutines to copy the data manually, and shifts the blocked Read into our own code. This lets us conditionally select on the copy goroutine finishing or the context expiring. @mitchellh: pushed a cleaner much fix to look at |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
There was still a race around the local-exec Command, where we were
calling Wait in 2 places which you can't do. There's no need for the
extra goroutine however, since we now have a context that we can pass to
the command itself.