Do not access uncompressed reader after close.
#609
Conversation
Before this commit, when the `applyDiff` function returned, the `defer uncompressed.Close()` was executed. This dealocated the resources which might still be used by another goroutine trying to read them. This commit fixes it by reusing existing `UploadReader` logic from the `image` package. Since the `UploadReader` is now used by both `image` and `storage`, it is now moved to `storage` and renamed to `TerminableReader` (it is no longer used just for "upload"). The `TerminableReader` wraps the `Read` function and if its `Terminate` is called, the `Read` function returns an error. This effectively ensures that the real closed `io.Reader` under the `TerminableReader` is never used after `close()` and the crash is fixed. Fixes containers#148. Signed-off-by: Jan Kaluza <jkaluza@redhat.com>
github-actions
bot
added
storage
|
The code should be ready, but I need to write tests for it. This might be hard, given it is a race condition between two goroutines... |
There was a problem hiding this comment.
I’m afraid I wrote the UploadReader suggestion originally — second thought, does that work?! There might be another bug in here.
We need the correct contents of tarSplitWriter, and for that, AFAICS the goroutine within NewInputTarStream must run fully to completion (observe tr.Next() == EOF, and write the last raw bytes in the paddingChunk loop).
So it’s not just that we need to observe EOF on the uncompressed stream being reached (something we could achieve by wrapping), we need to wait until that goroutine writes the last entries to storage.Packer.
And if that’s the case, I don’t think we can avoid changing NewInputTarStream. But that also makes it easier — we can have it write to a “done” channel, we can wait for that, and then the whole problem of closing uncompressed before the consumer is done might go away.
| } | ||
| defer uncompressed.Close() | ||
| var uncompressed_reader = terminablereader.NewTerminableReader(uncompressed) | ||
| defer uncompressed_reader.Terminate(errors.New("Reading data from an already terminated stream")) |
There was a problem hiding this comment.
(If this design persists: A comment somewhere about why we need the TerminableReader would be nice.)
|
I will do PR against tar-split. Doing it in wrapper is fragile and is more or less workaround for the real problem. |
3 participants
Before this commit, when the
applyDifffunction returned, thedefer uncompressed.Close()was executed. This dealocated the resources which might still be used by another goroutine trying to read them.This commit fixes it by reusing existing
UploadReaderlogic from theimagepackage. Since theUploadReaderis now used by bothimageandstorage, it is now moved tostorageand renamed toTerminableReader(it is no longer used just for "upload").The
TerminableReaderwraps theReadfunction and if itsTerminateis called, theReadfunction returns an error. This effectively ensures that the real closedio.Readerunder theTerminableReaderis never used afterclose()and the crash is fixed.Fixes #148.