code review 4629062: io: clarify Read, ReadAt, Copy, Copyn EOF behavior

io: clarify Read, ReadAt, Copy, Copyn EOF behavior

[rsc, 2011-06-22 19:57:48]

Hello golang-dev (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg

[bradfitz, 2011-06-22 20:07:05]

Could you look at the io.Copy and io.Copyn docs where you're in here?

When they return os.EOF has been a source of confusion for myself and others.

[rsc, 2011-06-22 20:56:49]

Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-06-22 21:06:10]

http://codereview.appspot.com/4629062/diff/5003/src/pkg/io/io.go
File src/pkg/io/io.go (right):

http://codereview.appspot.com/4629062/diff/5003/src/pkg/io/io.go#newcode256
src/pkg/io/io.go:256: // (including os.EOF), so can Copyn.
gross.

Copyn isn't an interface.  We control its implementation and there's just one.

I'd rather mask os.EOF and not return an error here so callers don't need to do
extra checks.  If I asked to copy n bytes and it copied n bytes, os.EOF isn't an
error as far as I'm concerned.

I don't want to sometimes get os.EOF and sometimes not.

http://codereview.appspot.com/4629062/diff/5003/src/pkg/io/io.go#newcode304
src/pkg/io/io.go:304: // A successful Copy returns err == nil, not err ==
os.EOF.
good.

[bradfitz, 2011-06-22 21:18:02]

(I know this was supposed to be a documentation-only CL, but I'm just
shocked at what the existing behavior apparently was...)

On Wed, Jun 22, 2011 at 2:06 PM, <bradfitz@golang.org> wrote:

>
>
http://codereview.appspot.com/**4629062/diff/5003/src/pkg/io/**io.go<http://c...
> File src/pkg/io/io.go (right):
>
> http://codereview.appspot.com/**4629062/diff/5003/src/pkg/io/**
>
io.go#newcode256<http://codereview.appspot.com/4629062/diff/5003/src/pkg/io/io.go#newcode256>
> src/pkg/io/io.go:256: // (including os.EOF), so can Copyn.
> gross.
>
> Copyn isn't an interface.  We control its implementation and there's
> just one.
>
> I'd rather mask os.EOF and not return an error here so callers don't
> need to do extra checks.  If I asked to copy n bytes and it copied n
> bytes, os.EOF isn't an error as far as I'm concerned.
>
> I don't want to sometimes get os.EOF and sometimes not.
>
> http://codereview.appspot.com/**4629062/diff/5003/src/pkg/io/**
>
io.go#newcode304<http://codereview.appspot.com/4629062/diff/5003/src/pkg/io/io.go#newcode304>
> src/pkg/io/io.go:304: // A successful Copy returns err == nil, not err
> == os.EOF.
> good.
>
>
>
http://codereview.appspot.com/**4629062/<http://codereview.appspot.com/4629062/>
>

[rsc, 2011-06-22 22:04:43]

> Copyn isn't an interface.  We control its implementation and there's
> just one.
>
> I'd rather mask os.EOF and not return an error here so callers don't
> need to do extra checks.  If I asked to copy n bytes and it copied n
> bytes, os.EOF isn't an error as far as I'm concerned.

The rationale is the same as for Read.
A correct caller has to handle the m < n, err != nil case,
and doing that correctly handles the m == n, err != nil case
trivially.

Russ

[iant, 2011-06-22 22:13:27]

FYI

http://codereview.appspot.com/4629062/diff/5003/src/pkg/io/io.go
File src/pkg/io/io.go (right):

http://codereview.appspot.com/4629062/diff/5003/src/pkg/io/io.go#newcode41
src/pkg/io/io.go:41: // successfully reading a n > 0 bytes, it must return the
number of
s/a n/n/

http://codereview.appspot.com/4629062/diff/5003/src/pkg/io/io.go#newcode41
src/pkg/io/io.go:41: // successfully reading a n > 0 bytes, it must return the
number of
The word "must" reads oddly to me here.  There is no real "must;" we're just
trying to describe what callers expect to see.  I think I would s/must/should/
here, or simply s/must return/returns/, but I understand why have written
"must."

http://codereview.appspot.com/4629062/diff/5003/src/pkg/io/io.go#newcode43
src/pkg/io/io.go:43: // or it can return the error (and n = 0) from a subsequent
call.
s/it can//
s/=/==/

http://codereview.appspot.com/4629062/diff/5003/src/pkg/io/io.go#newcode143
src/pkg/io/io.go:143: // When ReadAt returns n < len(p), it must return a
non-nil error
Another "must," to be fixed or left as-is per above.

http://codereview.appspot.com/4629062/diff/5003/src/pkg/io/io.go#newcode256
src/pkg/io/io.go:256: // (including os.EOF), so can Copyn.
On 2011/06/22 21:06:10, bradfitz wrote:
> If I asked to copy n bytes and it copied n bytes, os.EOF isn't an
> error as far as I'm concerned.
> 
> I don't want to sometimes get os.EOF and sometimes not.

This makes sense to me too.

[dsymonds, 2011-06-22 22:25:12]

On Thu, Jun 23, 2011 at 8:04 AM, Russ Cox <rsc@golang.org> wrote:

>> Copyn isn't an interface.  We control its implementation and there's
>> just one.
>>
>> I'd rather mask os.EOF and not return an error here so callers don't
>> need to do extra checks.  If I asked to copy n bytes and it copied n
>> bytes, os.EOF isn't an error as far as I'm concerned.
>
> The rationale is the same as for Read.
> A correct caller has to handle the m < n, err != nil case,
> and doing that correctly handles the m == n, err != nil case
> trivially.

I don't think it is. If io.Copyn only returned os.EOF if it does a
short copy, then my code can look like
  if _, err := io.Copyn(w, r, 42); err != nil {
    // handle error -- I know that I didn't get 42 bytes
  }
  // proceed

On the other hand, if io.Copyn may or may not return os.EOF for a
complete copy, then my code has to look like
  n, err := io.Copyn(w, r, 42)
  if err != nil && (err != os.EOF || n < 42) {
    // handle error -- either a short read, or another error
  }
  // proceed


Dave.

[rsc, 2011-06-22 22:29:44]

I don't have time to have this discussion this week.
This CL is about documenting the current behavior.

[nigeltao, 2011-06-22 22:55:06]

http://codereview.appspot.com/4629062/diff/5003/src/pkg/io/io.go
File src/pkg/io/io.go (right):

http://codereview.appspot.com/4629062/diff/5003/src/pkg/io/io.go#newcode35
src/pkg/io/io.go:35: // Even if Read returns n < len(p),
While you're here, this early line-break always struck me as odd-looking. Just
sayin'.

Same for ReaderAt at line 147.

</bikeshed>

[r, 2011-06-22 22:59:08]

LGTM after addressing iant's comments.
i agree that we can document what it does separately from redesigning the API

[dsymonds, 2011-06-22 23:12:34]

LGTM

I concur.

[rsc, 2011-06-22 23:20:18]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, iant@golang.org,
dsymonds@golang.org, nigeltao@golang.org, r@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2011-06-22 23:30:25]

LGTM

[rsc, 2011-06-22 23:33:09]

*** Submitted as http://code.google.com/p/go/source/detail?r=524ffaeede65 ***

io: clarify Read, ReadAt, Copy, Copyn EOF behavior

R=golang-dev, bradfitz, iant, dsymonds, nigeltao, r
CC=golang-dev
http://codereview.appspot.com/4629062