Read subprocess stderr with a blocking loop instead of readabilityHandler - #73
Conversation
| let stderrQueue = DispatchQueue(label: "\(name)-stderr-read-queue") | ||
| stderrQueue.async { | ||
| while true { | ||
| let data = orLog("Reading stderr from \(name)") { try stderrReadFileHandle.read(upToCount: 8 * 1024) } |
There was a problem hiding this comment.
Could we significantly reduce the amount of data we read here (maybe even 1 byte). Currently, if the server emits 60 bytes of stderr, it will not get reported until the pipe closes. Having the stderr logging close to the point when they were emitted can significantly help diagnosability of issues.
And since I don’t expect too much data to be sent on stderr, I don’t think that a small buffer size would be a big problem here. WDYT?
|
swiftlang/swift-corelibs-foundation#5507 The stress test passed. |
|
I think moving away from DispatchIO is a good move regardless, given how much trouble it has caused us in the past for stdin reading. |
d226499 to
faaf9cf
Compare
|
Reopening. |
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Great find. Just a few nitpicks inline
| // Reached end-of-file (0) or hit an unrecoverable error; stop reading. | ||
| break |
There was a problem hiding this comment.
Should we log an error with errno if an unrecoverable error was reached?
| /// A blocking read is used rather than a `readabilityHandler`. `readabilityHandler` installs a libdispatch | ||
| /// read source on the file descriptor; tearing that source down races epoll event delivery on Linux and | ||
| /// can crash the process in `_dispatch_event_loop_drain`. A blocking read never registers the descriptor | ||
| /// with libdispatch. |
There was a problem hiding this comment.
Worth referencing the corelibs-libdispatch issue here?
hamishknight
left a comment
hamishknight
left a comment
There was a problem hiding this comment.
LGTM modulo Alex's comments
faaf9cf to
114bcbc
Compare
`readabilityHandler` installs a libdispatch read source on the stderr pipe's file descriptor. Tearing that source down races epoll event delivery on Linux: `_dispatch_event_loop_drain` can dereference a `dispatch_muxnote` that a concurrent source teardown has already freed, crashing the process. This was observed intermittently in Linux CI while tearing down clangd connections and confirmed by core-dump analysis. Forward stderr with a blocking read loop on a dedicated queue instead. A blocking read never registers the file descriptor with libdispatch's event loop, so no read source is created for the pipe. Read the raw file descriptor rather than `FileHandle.read(upToCount:)`: on non-Darwin platforms the latter blocks until it accumulates the full requested count or reaches EOF, which would withhold small, intermittently emitted messages until the subprocess exits. A raw `read` returns as soon as any bytes are available. Extract the loop into `readInBackground` and add tests covering forwarding through end-of-file and prompt delivery of data before EOF.
114bcbc to
88c1a25
Compare
3 participants
JSONRPCConnection's subprocess launcher reads the child's stderr (e.g. clangd's) viaFileHandle.readabilityHandler, which installs a libdispatch read source on the pipe's file descriptor. Tearing that source down can race the descriptor being closed, which has intermittently crashed the process in_dispatch_event_loop_drainon Linux CI while a subprocess connection is torn down — a dangling epoll muxnote left behind by the source cancellation.This drains the subprocess's stderr with a blocking read loop on a dedicated queue instead. A blocking read never registers the file descriptor with libdispatch's event loop, so no read source (and no epoll muxnote) is created for the pipe. This mirrors the approach already used for the
receiveFDread loop in the same file, where the JSON-RPC input is read with blocking reads rather than a dispatch source. End of stream ends the loop; there is no asynchronous source cancellation to race the descriptor close.This is a speculative workaround for swiftlang/sourcekit-lsp#2717: the underlying defect is a teardown race in the runtime (Foundation
FileHandle/ libdispatch), tracked separately in swiftlang/swift-corelibs-foundation#5507. This PR removes the only libdispatch read source in the subprocess connection path —Processmonitoring on Linux uses CFRunLoop and the JSON-RPC pipes are already read with blocking reads — so it sidesteps the crash without changing observable behavior (stderr is still forwarded to the log).