fix: lazyBoundedReadCloser correctly close/seek #431
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark Test ResultsBenchmark results from the latest changes vs base branch |
Signed-off-by: Keith Zantow <[email protected]>
kzantow
force-pushed
the
fix/seekable-readers
branch
from
7493937 to
0b61951
Compare
spiffcs
reviewed
Jul 28, 2025
| if err != nil && errors.Is(err, io.EOF) { | ||
| // we've reached the end of the file, force a release of the file descriptor. If the file has already been | ||
| // closed, ignore the error. | ||
| if closeErr := d.file.Close(); !errors.Is(closeErr, os.ErrClosed) { |
Contributor
There was a problem hiding this comment.
Review note for self: d.Close() and d.file.Close() are different. d.Close() does not hoist from d.file.Close(). It's using: https://github.com/anchore/stereoscope/blob/main/pkg/file/lazy_bounded_read_closer.go#L53-L66
d.Close() calls d.file.Close() while also resetting d.file and d.reader to nil
closeErr cannot be os.ErrClosed given this err type is already accounted for by d.Close() and ignored.
@kzantow what was the original issue where you saw this behave weird when it wasn't reset?
I just want to know how this manifested in the wild so I can understand the change before 🟢
Contributor
Author
There was a problem hiding this comment.
The problem is evident in syft main branch, JDK binary detection against an image. For example:
go run ./cmd/syft ibmjava:8-jre@sha256:3588cd1cc9b8646fe03b3b15210e69b1b520f1321f8518b69c0e7013d702fd23 | grep '^java '
... returns no results. The reason for this is the java binary is read multiple times, calling .Seek here but stereoscope maintains a reference to the closed file, preventing further .Seek calls from opening it again, whereas d.Close sets this to nil, resulting in the next d.openFile() call to actually open the file again for reading. I don't think the error returned from Close should be returned, because this only occurs when EOF is returned and if EOF is not returned to the original caller, there are cases it may not proceed or retry and get in an infinite loop, etc..
Tests are passing in syft because they are using a directory source, not an image source. 😢
With this change, you should see:
go run ./cmd/syft ibmjava:8-jre@sha256:3588cd1cc9b8646fe03b3b15210e69b1b520f1321f8518b69c0e7013d702fd23 | grep '^java '
...
java 1.8.0-_2025_04_14_02_37-b00 binary
Contributor
There was a problem hiding this comment.
Awesome - was able to reproduce on my end with the new change. TY for the walk through.
I don't think the error returned from Close should be returned, because this only occurs when EOF is returned and if EOF is not returned to the original caller, there are cases it may not proceed or retry and get in an infinite loop, etc..
Hard agree on the above change is 🟢 for me.
kzantow
commented
Jul 28, 2025
| // we've reached the end of the file, release of the file descriptor. continue to return EOF | ||
| // IMPORTANT: call d.Close to reset internal state | ||
| if closeErr := d.Close(); closeErr != nil { | ||
| log.Tracef("unable to close: %v: %v", d.path, closeErr) |
Contributor
Author
There was a problem hiding this comment.
I'm not entirely sure the best behavior here, can certainly be convinced to change this... but this is only hit if we received EOF during the Read call, which will be returned to the caller and we're managing open file handles behind the scenes, so while an error closing here can indicate a problem, there's not much that a caller can do about this if it fails and they are already receiving an EOF, so should stop further reading anyway unless they Seek, at which time we need to be able to reopen the file and a serious problem like the file being removed will cause an error then, more appropriately, etc.
spiffcs
approved these changes
Jul 29, 2025
wagoodman
approved these changes
Jul 29, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes an issue where reading a lazyBoundedReadCloser is unable to seek due to being closed but not reset properly. In other words:
.Seek(0, io.SeekStart)attempts to calld.openFile(), but since there is a non-nil reference to a reader, it does not reopen the file but fails to seek and read from it afterwards instead of actually reopening it for reading.