fix(build): stop leaking the splitReader goroutine when an interceptor abandons its reader#512
fix(build): stop leaking the splitReader goroutine when an interceptor abandons its reader#512SAY-5 wants to merge 2 commits intomodelpack:mainfrom
Conversation
…r abandons its reader splitReader fans out a source io.Reader into two pipe-backed readers with a single io.Copy goroutine feeding a MultiWriter. io.Pipe writes are synchronous, so if either returned reader is abandoned without being drained to EOF - the interceptor returning an error early is the concrete case in modelpack#492 - MultiWriter.Write blocks, io.Copy never returns, and the goroutine leaks for the lifetime of the process. That also deadlocks the other consumer, because the second pipe writer is wedged behind the first. Plumb the caller's context into splitReader and run a parallel watcher that calls CloseWithError on both pipe writers when ctx is canceled. The copy goroutine then observes the error, io.Copy returns, both pipes close. The only caller (adapterBuilder.storeLayer inside build/builder.go) already has ctx in scope and passes it through. Fixes modelpack#492 Signed-off-by: SAY-5 <[email protected]>
There was a problem hiding this comment.
Code Review
This pull request updates the splitReader function to accept a context, which is used to prevent goroutine leaks by closing pipe writers if the context is cancelled. While this improves resource management, feedback indicates that BuildLayer still contains a potential goroutine leak if OutputLayer fails, as the function can return early without waiting for the interceptor goroutine to complete.
| if ab.interceptor != nil { | ||
| var itReader io.Reader | ||
| reader, itReader = splitReader(reader) | ||
| reader, itReader = splitReader(ctx, reader) |
There was a problem hiding this comment.
While passing ctx to splitReader helps reclaim the goroutine when the parent context is cancelled, BuildLayer still has a potential goroutine leak if OutputLayer fails (line 191). In that case, the function returns early without waiting for the interceptor goroutine (wg.Wait() is at line 197 and is skipped). Since OutputLayer likely stopped reading from reader, the splitReader goroutine will block on w1.Write, and the interceptor will block on itReader.Read. These will only be reclaimed when the parent ctx is cancelled, which might be much later in a long-running process. Consider using a scoped context with cancellation and ensuring wg.Wait() is called on all exit paths to fully address the leak when OutputLayer fails.
…r) signature
The earlier commit on this branch added context.Context as the first
argument of splitReader so the copy goroutine can unblock on ctx.Done().
TestPipeReader still passed only the reader, which tripped the lint/CI
typecheck:
pkg/backend/build/builder_test.go:287:24: not enough arguments in call to splitReader
have (*strings.Reader)
want (context.Context, io.Reader)
Pass context.Background() so the test exercises the new signature.
`context` is already imported by other tests in this file.
What
splitReaderinpkg/backend/build/builder.gofans out a sourceio.Readerinto two pipe-backed readers with a singleio.Copygoroutine feeding aMultiWriter:io.Pipewrites are synchronous: a write blocks until the pairedPipeReaderconsumes the bytes. If either returned reader is abandoned without being drained to EOF — the interceptor returning early on error is the concrete case flagged in #492 —MultiWriter.Writeblocks insideio.Copy, the goroutine never returns, and it leaks for the lifetime of the process. Worse, the other consumer also deadlocks because the write into the second pipe is wedged behind the first.Fix
Plumb the caller's context into
splitReaderand run a parallel watcher that callsCloseWithErroron both pipe writers on ctx cancel:The copy goroutine observes the
ErrClosedPipefromCloseWithError,io.Copyreturns, both pipes close cleanly. The parallel watcher goroutine itself exits through thecopyDonesignal (always closed via defer) so it doesn't leak either.The only caller (
adapterBuilder.storeLayer) already hasctxin scope and passes it through.Fixes #492