limitedReadCloser, introduced in #2271, releases its slot only on Close(). Because io.Copy reads to EOF and returns without calling Close(), any caller that fails to explicitly Close() holds a limiter slot per layer.
Some deadlocks within this repo were caught after v0.21.6 and fixed:
| Issue |
Caller |
Fix |
| #2276 |
mutate.Extract (crane export) |
#2277 |
| #2309 |
tarball.MultiRefWrite (crane pull) |
#2308 |
| #2342 |
internal/gzip.ReadCloserLevel (kaniko via layout.WriteImage) |
#2347 |
Each fix correctly adds defer r.Close().
External consumers of Compressed() are in the same position. For example, carvel-dev/imgpkg calls layer.Compressed() in a loop in imagetar/tar_writer.go, passes the result to io.Copy, and never closes on success. This is structurally similar to the fix in #2308. imgpkg deadlocks on any bundle with more than four layers when using go-containerregistry ≥ v0.21.6.
While these missing Close() calls were arguably bugs on their own, it would be better if the consequence were not a deadlock, just briefly leaked resources. Although Compressed returns an io.ReadCloser, the contract of the method should probably not be to expect a deadlock if the consumer fails to call Close on all paths.
Possible improvement to limitedReadCloser: Once Read() returns EOF (or any other non-nil error), the download is no longer in flight and the slot should be freed. Adding a Read() override to limitedReadCloser would fix this for all current and future consumers without breaking any existing caller:
func (l *limitedReadCloser) Read(p []byte) (int, error) {
n, err := l.ReadCloser.Read(p)
if err != nil {
l.once.Do(l.release)
}
return n, err
}
limitedReadCloser, introduced in #2271, releases its slot only onClose(). Becauseio.Copyreads to EOF and returns without callingClose(), any caller that fails to explicitlyClose()holds a limiter slot per layer.Some deadlocks within this repo were caught after v0.21.6 and fixed:
mutate.Extract(crane export)tarball.MultiRefWrite(crane pull)internal/gzip.ReadCloserLevel(kaniko vialayout.WriteImage)Each fix correctly adds
defer r.Close().External consumers of
Compressed()are in the same position. For example, carvel-dev/imgpkg callslayer.Compressed()in a loop inimagetar/tar_writer.go, passes the result toio.Copy, and never closes on success. This is structurally similar to the fix in #2308.imgpkgdeadlocks on any bundle with more than four layers when using go-containerregistry ≥ v0.21.6.While these missing
Close()calls were arguably bugs on their own, it would be better if the consequence were not a deadlock, just briefly leaked resources. AlthoughCompressedreturns anio.ReadCloser, the contract of the method should probably not be to expect a deadlock if the consumer fails to callCloseon all paths.Possible improvement to
limitedReadCloser: OnceRead()returns EOF (or any other non-nilerror), the download is no longer in flight and the slot should be freed. Adding aRead()override tolimitedReadCloserwould fix this for all current and future consumers without breaking any existing caller: