Skip to content
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

io.go: added Seek() to implement io.ReadSeeker #17

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Gewinum
Copy link

@Gewinum Gewinum commented Aug 15, 2024

I thought about adding the whole ReadSeeker class but didn't want to simply copy-paste code so I made this. Needed this for http.ServeContent

Copy link
Member

@raulb raulb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Gewinum, thanks for the contribution! I'd like to defer to @lovromazgon for an official review, but as part of the @ConduitIO/conduit-core team, I'd like to chime in in the meantime. Things that come to mind:

  1. The Read method makes use of the limiter and I think it'd be interesting to replicate that feature making use the same limiter.
  2. This Seek method is exported and needs to be documented.
  3. This new method has no tests on this PR, and in order to make it ready for review, this needs to be tested.

Again, thank you for the contribution. I really appreciate it!

@lovromazgon
Copy link
Member

The fact that this method returns an error makes me think we should not add it, let me explain.

It's not uncommon for functions to check if a io.Reader implements another interface which provides more functionality, so the function can make use of that functionality. So I don't think it's far fetched if a function would check if a reader implements io.ReadSeeker like this:

func Foo(r io.Reader) error {
	rs, ok := r.(io.ReadSeeker)
	if ok {
		// this is an io.ReadSeeker, do something differently
		i, err := rs.Seek(offset, whence)
		if err != nil {
			return err // whoops, failed to seek
		}
		...
	} else {
		// normal reader, general case
		...
	}
}

Notice that once we drop into the first branch of the if statement the code is convinced it has a valid io.ReadSeeker and an error returned from Seek would indicate that the seek failed. The same code would work if the reader wouldn't implement io.ReadSeeker, as it would drop into the second branch. So given that we can't be sure that the underlying reader actually implements io.ReadSeeker, I don't think we should add the method and advertise the reader as an io.ReadSeeker.

A workaround for this would be for you to implement your own type and make sure you construct it only if you know the underlying reader is a io.ReadSeeker:

type LimitedReadSeeker struct {
	bwlimit.Reader
}

func (r *LimitedReadSeeker) Seek(offset int64, whence int) (int64, error) {
	seekR, ok := r.Reader.(io.ReadSeeker)
	if !ok {
		return 0, errors.New("reader must be seeker")
	}
	return seekR.Seek(offset, whence)
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants