Skip to content

Make NewCustomCommitWriteCloser preserve io.Seeker interface#11279

Merged
bduffany merged 2 commits intomasterfrom
commit-seeker
Feb 10, 2026
Merged

Make NewCustomCommitWriteCloser preserve io.Seeker interface#11279
bduffany merged 2 commits intomasterfrom
commit-seeker

Conversation

@bduffany
Copy link
Member

@bduffany bduffany commented Feb 6, 2026

Fixes a bug that certain bytestream reads are not retryable, e.g. this one:

w := ioutil.NewCustomCommitWriteCloser(f)

Copy link
Contributor

@vanja-p vanja-p left a comment

Choose a reason for hiding this comment

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

I like this in general, partially because I've wanted to make CustomCommitWriteCloser keep the ReadFrom method if it's there. On the other hand, if we wanted to keep ReadFrom as well, we would need 4 implementations of customCommitWriteCloser.

Here are 2 alternatives:

  1. Since Seek returns an error, could the existing, concrete CustomCommitWriteCloser always have a Seek method, and return an error if the underlying writer doesn't support it? We would have to change cachetools.GetBlob to not do the initial Seek.
  2. We could have a ioutil.GetSeeker function, which accepts x any. If x is an io.Seeker it returns that. If not, it does a type match against all the concrete types in ioutil, and returns the underlying reader or writer if it implements io.Seeker? Then cachetools.GetBlob (and UploadFromReaderWithCompression) would call that instead of doing a type check.

@bduffany
Copy link
Member Author

bduffany commented Feb 7, 2026

@vanja-p I think we should optimize for reduced complexity of the ioutil package interface, rather than reduced complexity of the ioutil package implementation, because it is such a high-utility interface with lots of callers.

  • Alternative 1 adds complexity to callers because they have to remember to check both whether something is an io.Seeker and also check whether a seek returns an ignorable error.
  • Alternative 2 adds complexity to callers because they have to remember to call GetSeeker instead of doing a simple cast to io.Seeker.

if we wanted to keep ReadFrom as well, we would need 4 implementations of customCommitWriteCloser.

I prefer having 4 implementations to both of the above alternatives, since the complexity is hidden in the ioutil package.

Copy link
Contributor

@vanja-p vanja-p left a comment

Choose a reason for hiding this comment

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

I have a few other comments pending, but SGTM.

@bduffany bduffany force-pushed the commit-seeker branch 2 times, most recently from 0a07bab to 9c44811 Compare February 9, 2026 18:44
@bduffany
Copy link
Member Author

bduffany commented Feb 9, 2026

@vanja-p I addressed your concerns in c8926ae

@bduffany bduffany merged commit 3e8c8c9 into master Feb 10, 2026
11 of 13 checks passed
@bduffany bduffany deleted the commit-seeker branch February 10, 2026 15:25
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