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.

Sign up for GitHub

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

Jump to bottom

proposal: io: "unimplemented" error return for WriterTo et al. #21904

Closed
neild opened this issue Sep 15, 2017 · 6 comments
Closed

proposal: io: "unimplemented" error return for WriterTo et al. #21904

neild opened this issue Sep 15, 2017 · 6 comments
Labels
FrozenDueToAge Proposal v2 A language change or incompatible library change
Milestone
Proposal

Comments

@neild
Copy link
Contributor

neild commented Sep 15, 2017

Previous discussion: #21670 (comment)

io.Copy takes an io.Reader, but uses a type assertion to select an alternate implementation if the reader implements io.WriterTo. This pattern makes it difficult to write a function which wraps an arbitrary reader--if the wrapper just wraps the Read method it erases the more efficient path implemented by WriteTo, but there's no simple way to wrap the WriteTo method only if present.

There are a variety of features we might add to the language to better support this case, but there's a simpler one which doesn't require any language changes at all: io.Copy and other functions which probe for an optional method shouldn't assume that a type has a working version of a method just because it implements it.

Concretely, this is a proposal that the start of io.Copy be changed to something like this:

func Copy(dst Writer, src Reader) (written int64, err error) {
  // If the reader has a WriteTo method, use it to do the copy.
  // Avoids an allocation and a copy.
  if wt, ok := src.(WriterTo); ok {
    if written, err := wt.WriteTo(dst); err != ErrUnimplemented {
      return written, err
    }
  }
  /* ... */
}

If WriteTo returns an error indicating that the method is unimplemented, Copy will continue along as if that method were not present.

Searching for non-test type assertions to ReaderFrom and WriterTo in the stdlib, I find:

There might be some other methods that should get the same treatment.
@gopherbot gopherbot added this to the Proposal milestone Sep 15, 2017
@dsnet
Copy link
Member

dsnet commented Sep 15, 2017 •
edited

Related to #16474 where I noted that there are many inefficient implementations of io.ReaderFrom and io.WriterTo that just call io.Copy blindly under the hood.

Another example is io.Seeker, where there are types that have the method, but do not implement it. In archive/tar, it performs a hack where it tries Seek(0, io.SeekCurrent) to test if the method is supported.

go/src/archive/tar/reader.go

Lines 682 to 694 in c167928

// Not all io.Seeker can actually Seek. For example, os.Stdin implements
// io.Seeker, but calling Seek always returns an error and performs
// no action. Thus, we try an innocent seek to the current position
// to see if Seek is really supported.
pos1, err := sr.Seek(0, io.SeekCurrent)
if pos1 >= 0 && err == nil {
// Seek seems supported, so perform the real Seek.
pos2, err := sr.Seek(n-1, io.SeekCurrent)
if pos2 < 0 || err != nil {
return 0, err
}
seekSkipped = pos2 - pos1
}

This test is necessary since os.Stdin is an os.File which has File.Seek, but oftentimes cannot actually seek because the input is from a stream.

@dsnet
Copy link
Member

dsnet commented Sep 15, 2017

I should note that detecting whether a method is truly implemented or not requires actually calling that method, which may have side-effects (in the case of io.{ReaderFrom,WriterTo}. In other words, there is no way to first query if the method is implemented, and then make a programmatic decision on what to do.

However, in all of the use cases I have personally seen, the method that is being attempted is always the first choice, and if it were not implemented, then there is a fallback.

@jimmyfrasche
Copy link
Member

Personally I don't want to have to always have to ask everything whether it was lying when it said it could do something.

Especially since the root problem of extending an arbitrary interface value also effects the proposed sentinel error so everything would always have to be careful to never add any extra context to it or code testing for the sentinel wouldn't get triggered.

@dsnet dsnet changed the title proposal: "unimplemented" error return for io.WriterTo et al. proposal: io: "unimplemented" error return for io.WriterTo et al. Sep 16, 2017
@dsnet dsnet changed the title proposal: io: "unimplemented" error return for io.WriterTo et al. proposal: io: "unimplemented" error return for WriterTo et al. Sep 16, 2017
@neild
Copy link
Contributor Author

neild commented Sep 18, 2017

Especially since the root problem of extending an arbitrary interface value also effects the proposed sentinel error so everything would always have to be careful to never add any extra context to it or code testing for the sentinel wouldn't get triggered.

This is also an issue with io.EOF et al. The general question of how to produce distinguishable errors that can be wrapped is an interesting one, but I'd hate for us to give up on producing identifiable errors entirely until a perfect solution can be found.

@rsc rsc added the v2 A language change or incompatible library change label Sep 18, 2017
@rsc
Copy link
Contributor

rsc commented Sep 18, 2017

I see what you are saying, and we did learn that lesson for some other of the optional methods, like IsBoolFlag in package flag, or net.Error, but probably it doesn't make sense to redesign parts of package io in this way until we get to a comprehensive review of io as part of Go 2.

@dsnet dsnet mentioned this issue Jan 2, 2018
Open
@ianlancetaylor
Copy link
Contributor

I think this is not the right approach, as described at #16474 (comment) . I'm going to close this issue in favor of the discussion over there.

@bcmills bcmills mentioned this issue Sep 14, 2018
Closed
@bcmills bcmills mentioned this issue Dec 20, 2018
Open
@golang golang locked and limited conversation to collaborators Jan 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal v2 A language change or incompatible library change
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
6 participants
@neild @rsc @jimmyfrasche @ianlancetaylor @dsnet @gopherbot