mirror of
https://github.com/apple/container.git
synced 2026-09-23 16:15:37 +00:00
Tests/CLITests: drain stdout/stderr concurrently in CLITest.run (#1471)
## Type of Change - [x] Bug fix - [ ] New feature - [ ] Breaking change - [ ] Documentation update ## Motivation and Context `CLITest.run(arguments:stdin:currentDirectory:env:)` reads stdout to EOF before stderr begins draining (`Tests/CLITests/Utilities/CLITest.swift:182-183`). If a child process writes more than a pipe-buffer's worth of data to stderr (~64 KB on macOS), the child blocks in `write()` on stderr while we block in `readDataToEndOfFile()` on stdout, and neither side makes progress until `process.waitUntilExit()` returns. This is a latent deadlock that any future test or CLI verbosity bump can trigger. This PR drains both streams concurrently using `readabilityHandler` closures backed by `Mutex<Data>` buffers. After `process.waitUntilExit()` returns the handlers are cleared and `readDataToEndOfFile()` flushes any bytes the kernel buffered between the last handler invocation and exit. The error path also clears handlers so a failed `process.run()` does not leak callbacks. `Mutex` matches the locking primitive the file already uses for `commandSeq`. Fixes #1456 ## Testing - [x] Tested locally - [ ] Added/updated tests - [ ] Added/updated docs `swift build --target CLITests` passes against the change. `make swift-fmt-check` is clean. Behavior is unchanged for any test command that previously fit within a single pipe buffer, so the existing CLITests still exercise the helper end-to-end. I did not add a new test that emits >64 KB to stderr because every existing CLITest invocation goes through `executablePath` (the `container` binary), and reproducing the deadlock requires a child that emits a controllable amount on stderr. Happy to follow up with a small refactor that extracts the drain into a static helper plus a regression test that drives it via `/bin/sh` if that would be useful. --------- Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
This commit is contained in:
co-authored by
Matt Van Horn
parent
f9899013fd
commit
b73933eb2d
@@ -165,23 +165,41 @@ class CLITest {
|
||||
}
|
||||
|
||||
let inputPipe = Pipe()
|
||||
let outputPipe = Pipe()
|
||||
let errorPipe = Pipe()
|
||||
process.standardInput = inputPipe
|
||||
process.standardOutput = outputPipe
|
||||
process.standardError = errorPipe
|
||||
|
||||
let outputData: Data
|
||||
let errorData: Data
|
||||
do {
|
||||
// Redirect stdout/stderr to temp files so the child process never
|
||||
// blocks on `write()` when one stream fills the kernel pipe buffer
|
||||
// before the parent drains it (issue #1456).
|
||||
let tempDir = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString)
|
||||
try FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true)
|
||||
defer {
|
||||
try? FileManager.default.removeItem(at: tempDir)
|
||||
}
|
||||
|
||||
let stdoutURL = tempDir.appendingPathComponent("stdout")
|
||||
let stderrURL = tempDir.appendingPathComponent("stderr")
|
||||
FileManager.default.createFile(atPath: stdoutURL.path, contents: nil)
|
||||
FileManager.default.createFile(atPath: stderrURL.path, contents: nil)
|
||||
|
||||
let stdoutHandle = try FileHandle(forWritingTo: stdoutURL)
|
||||
defer { try? stdoutHandle.close() }
|
||||
let stderrHandle = try FileHandle(forWritingTo: stderrURL)
|
||||
defer { try? stderrHandle.close() }
|
||||
process.standardOutput = stdoutHandle
|
||||
process.standardError = stderrHandle
|
||||
|
||||
try process.run()
|
||||
if let data = stdin {
|
||||
inputPipe.fileHandleForWriting.write(data)
|
||||
}
|
||||
inputPipe.fileHandleForWriting.closeFile()
|
||||
outputData = outputPipe.fileHandleForReading.readDataToEndOfFile()
|
||||
errorData = errorPipe.fileHandleForReading.readDataToEndOfFile()
|
||||
process.waitUntilExit()
|
||||
|
||||
outputData = try Data(contentsOf: stdoutURL)
|
||||
errorData = try Data(contentsOf: stderrURL)
|
||||
} catch {
|
||||
throw CLIError.executionFailed("Failed to run CLI: \(error)")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user