commands: stop dial-stdio when the builder connection closes#3790
commands: stop dial-stdio when the builder connection closes#3790tonistiigi merged 1 commit intodocker:masterfrom
Conversation
|
This kind of leaves the reader in unknown state. Normally you would read to |
Signed-off-by: CrazyMax <[email protected]>
I simplified this by removing the cancelable reader wrapper entirely. The proxy now starts the two When stdin finishes first, it closes the connection write side and keeps waiting for remote output. When the remote side finishes first, it closes the read side and returns immediately, so That avoids leaving the original reader in an odd state, since we no longer put a pipe reader in front of it or try to cancel it from the outside. I also removed the extra I tested it on a real remote builder by closing the connection. |
cpuguy83
left a comment
There was a problem hiding this comment.
Sorry, I don't recall this one coming in.
LGTM.
| if err != nil && !errors.Is(err, net.ErrClosed) && !errors.Is(err, io.ErrClosedPipe) { | ||
| return err | ||
| } | ||
| stdinDone = nil |
fixes #3668
Fixes a
dial-stdiohang that could persist after the builder connection closed. The command now exits when the read side finishes instead of waiting for a new write on stdin to unblock the process.The old code used two
io.Copycalls and waited for both to finish, which meant a blocked read from stdin could keep the command alive even after the builder connection had already died. This showed up when the Docker daemon restarted anddial-stdiostayed hung until the user pressed Enter.Tried to add an integration test for this but that's tricky. We would need some wiring with explicit restart/kill hook into the worker to test this regression. So needs changes in BuildKit test framework imo.
cc @invidian @cpuguy83