-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[bug] - Improve seekability check for stdout pipes in BufferedReadSeeker #3189
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has a few changes that don't appear related to the described fix - are they actually related?
2617930
to
642e0ed
Compare
Removed. Will add to a separate PR. |
Added here |
seeker io.Seeker // If the reader supports seeking, it's stored here for direct access | ||
isSeekable bool // Indicates if the reader supports reliable seeking operations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to remove isSeekable
in favor of checking seeker != nil
?
pkg/iobuf/bufferedreaderseeker.go
Outdated
if isSeekable(r) { | ||
seeker, _ = r.(io.Seeker) | ||
seekable = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's only used here, I'd suggest changing isSeekable
to:
func asSeeker(r io.Reader) io.Seeker { /* ... */ }
and using it like:
if s := asSeeker(r); s != nil {
seeker = s
} else {
buf = defaultBufferPool.Get()
}
or maybe
seeker = asSeeker(r)
if seeker == nil {
buf = defaultBufferPool.Get()
}
seeker = asSeeker(r) | ||
if seeker == nil { | ||
isSeekable = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isSeekable
should be set to false
, correct? But what do you think about removing isSeekable
altogether in favor of seeker != nil
checks?
Description:
When handling files over stdout.Pipe, our current method of checking if a reader is seekable using a simple interface assertion is insufficient. The underlying type of the response from stdout.Pipe is an os.File, which typically implements the io.Seeker interface. However, in this case, it cannot be treated like a regular file and doesn't support seeking operations.
Checklist:
make test-community
)?make lint
this requires golangci-lint)?