fix(dist): close piped stdin before waiting on the child - #2806
fix(dist): close piped stdin before waiting on the child#2806oguzhanmeteozturk wants to merge 1 commit into
Conversation
`CommandExt::check_piped` moves the child's stdin out of the `Child` with `.take()`, binding it to a local that lives until the end of the function. `Child::wait_with_output` closes only the stdin still held by the `Child` (`drop(self.stdin.take())`), which is `None` here, so that is a no-op and the write end of the pipe stays open while the parent waits for the child to exit. The closure cannot close it either: its parameter is `&mut ChildStdin`, so the callee has no way to drop the value. Only `check_piped` can. Both callers are in `DockerBuilder` and pipe a tar into `docker cp - <container>:/`, which reads until EOF. With the write end held open it never exits, so every job on a `type = "docker"` build server blocks indefinitely with its container left in `Created` and the client falls back to local compilation. Both call sites already drop the corresponding reader immediately after the call (`drop(toolchain_rdr)`, `drop(inputs_rdr)`); this makes the writer symmetric.
|
it will need a test to make sure it won't regress please make comment #0 a bit shorter in the future |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2806 +/- ##
==========================================
- Coverage 73.14% 73.12% -0.02%
==========================================
Files 72 72
Lines 37615 37592 -23
==========================================
- Hits 27514 27490 -24
- Misses 10101 10102 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Writing the test exposed a gap: all nine test_dist_* tests go through DistSystem::add_server, which hardcodes BuilderType::Overlay. Nothing ever sets type = "docker", so the Docker builder had no coverage. sccache_server_cfg now takes a BuilderType, there’s a new add_docker_server() that mounts the daemon socket, however the harness image (Dockerfile.sccache-dist) will need the Docker client. Happy to push it |
Fixes #2805
What
CommandExt::check_pipeddrops theChildStdinit took from theChildbeforecalling
wait_with_output(), so the child sees EOF on stdin.pipe(&mut stdin).context("Failed to pipe input to process")?; + // `process.stdin` was moved out by the `.take()` above, so + // `wait_with_output()` has no stdin left to close. Without this the write + // end stays open for the rest of the function and a child that reads to EOF + // never returns. + drop(stdin); let output = process .wait_with_output() .context("Failed to wait for process to return")?;Why
check_pipedtakes stdin out of theChild:stdinis owned and lives until the end of the function, so it is still openwhile
wait_with_output()blocks.Child::wait_with_outputcloses only thestdin still held by the
Child(drop(self.stdin.take())), which isNonehere, making that a no-op.
The closure cannot close it either — its parameter is
&mut ChildStdin, so thecallee cannot drop the value. Only
check_pipedcan.The two callers are
DockerBuilder::make_image(build.rs:720) andDockerBuilder::perform_build(build.rs:771), both of which pipe a tar intodocker cp - <container>:/.docker cp -reads until EOF, so with the write endheld open it never exits, and every job on a
type = "docker"build serverblocks indefinitely with its container left in
Created.Both call sites already
drop()the corresponding reader right after the call(
drop(toolchain_rdr),drop(inputs_rdr)); this makes the writer symmetric.Reproducing the underlying behaviour
That
docker cp -requires EOF rather than merely a complete archive can beshown without sccache:
The archive is complete and valid in both cases; only the writer's behaviour
differs. Measured on Docker 29.7.2, and reproduced on an earlier client, so this
is not specific to a particular release.
Testing
sccache gcc -c hello.cagainst atype = "docker"buildserver made no progress; the container stayed in
Createdand the client fellback to local compilation.
defconfigbuild distributed 2,867 compilationsacross 11 build servers with no scheduler allocation failures.
Scope
One statement, in a helper used only by the Docker builder. The
overlayandpotbuilders do not callcheck_piped. No public API or configuration change.