doc: Effective Go has a bad/unreliable idiom

[seebs]

What version of Go are you using (go version)?

1.11

Does this issue reproduce with the latest release?

N/A, but yes

What operating system and processor architecture are you using (go env)?

N/A, but linux/amd64

What did you do?

Read "Effective Go" again. Specifically, the slice example:

    var n int
    var err error
    for i := 0; i < 32; i++ {
        nbytes, e := f.Read(buf[i:i+1])  // Read one byte.
        if nbytes == 0 || e != nil {
            err = e
            break
        }
        n += nbytes
    }

What did you expect to see?

A good example of Effective Go. :)

What did you see instead?

A program which will behave incorrectly in the fairly common case where an io.Reader can return a non-fatal or "benign" error along with a non-zero byte count. iotest.DataErrReader does this explicitly, but I believe at least one of the commonly-used Reader interfaces does it already; the Read which reaches EOF can return io.EOF along with a full read.

suggested fix: just drop the || e != nil; short reads are impossible when the requested number of bytes is 1. If nbytes is 0, an error must have occurred and we don't need to check whether e is nil, and if nbytes is 1, e can be non-nil without implying that there's a problem. (It would usually be io.EOF in that case, so a break is still reasonable, but setting err may be misleading. Usually if the read is successful, an io.EOF gets ignored rather than bubbling up as an error state.)

[gopherbot]

Change https://golang.org/cl/140957 mentions this issue: doc: modernize slice read example in Effective Go

[RalphCorderoy]

The loop follows n, err := f.Read(buf[0:32]) and is presumably trying to ape it so io.EOF shouldn't be ignored, but end up in err. Similarly, a transient read problem might cause the single Read of 32 bytes to return 17 and non-nil and the single-byte version can do the same. Isn't the only problem that nbytes isn't always added to the n total?

var n int
var err error
for i := 0; i < 32; i++ {
    var nbytes int
    nbytes, err = f.Read(buf[i:i+1]) // Read one byte.
    n += nbytes
    if err != nil {
        break
    }
}

[seebs]

I think that's correct. I've tried several times to write a thing that's both correct and clear. I had come up with

        if nbytes < 1 {
            err = e
            break
        }
        n += nbytes

but then I feel like it makes sense to comment on the rationale for the break, and that takes more space and clutters the example.

I think your solution is clean as self-contained code, but it violates the same Principle of Least Astonishment that iotest.DataErrReader violates, and yields an error when there's no problem; it's fairly idiomatic in Go to assume that an error indicates that there is a problem, and that you should treat an operation which returns an error as having failed.

It gets worse if you even consider trying to handle things like iotest.TimeoutReader.

I've been testing it with a test harness of sorts on Playground:

https://play.golang.org/p/uVJctuzC9AX

The intent being that it should get all 32 bytes for both "easy" and "hard", but correctly report the error for "fail".

In any event, it does stay annoyingly difficult.

My best attempt at commenting the nbytes < 1 form I was experimenting with:

(Errors are ignored unless nbytes is less than the requested 1, because some Read functions will return an EOF on the successful read of the last byte from a stream.)

... note also that the "nbytes < 1" test is exploiting the fact that you can't get a short read without some kind of error, and there's no partial fills of a single-byte read. Otherwise, it would still be necessary to move n += nbytes up. Which I think maybe argues that moving it up is the right choice regardless.

[RalphCorderoy]

I think your solution is clean as self-contained code, but it violates the same Principle of Least Astonishment... and yields an error when there's no problem

It stops reading on an error and sets err, as the original Read(32) would do. Why should we try to do more than the Read(32) that immediately precedes this in the text?

it's fairly idiomatic in Go to assume that an error indicates that there is a problem, and that you should treat an operation which returns an error as having failed.

Yes, an error indicates a problem, but for reading it doesn't mean it's failed. The caller of Read(32) knows the context and can decide what's a failure. Perhaps 17, EOF is failure, perhaps not. The Read(32)-as-a-loop expansion shouldn't be trying to embed this caller's knowledge IMO as it didn't follow the Read(32) either, and making up possible context and dealing with it just clutters.

Given an io.Reader may return 0,nil and it's to documented as a no-op, I think the fault with the Read(1) loop is it doesn't n += nbytes before the conditional-break test. It's also arguable whether a pedagogical example should be checking n == 0, encouraging it to be considered as EOF. io.ErrNoProgress exists for multiple 0,nil returns, but it's not the example loop's place to produce that.

The text leading to the loop says 'read the first 32 bytes of the buffer' and could do with an 'up to'.

[RalphCorderoy]

This handles a no-op return from Read.

var n int 
var err error
for n < 32 {
    var got int 
    got, err = f.Read(buf[n : n+1]) // Read one byte.
    n += got                        // 0, nil from Read is a no-op.
    if err != nil {
        break
    }
}   

[seebs]

Wait, Reader can return {0,nil}? I thought... huh. Okay, re-reading this, I am now very confused, I was pretty sure that it was mandated that err had to be non-nil if n was less than the length. But it's not!

Callers should always process the n > 0 bytes returned before considering the error err. Doing so correctly handles I/O errors that happen after reading some bytes and also both of the allowed EOF behaviors.

Implementations of Read are discouraged from returning a zero byte count with a nil error, except when len(p) == 0. Callers should treat a return of 0 and nil as indicating that nothing happened; in particular it does not indicate EOF.

So I was just plain wrong on that. I am probably too used to POSIX semantics, in which a return of exactly 0 bytes is EOF, because an error that wasn't EOF and prevented any reading would be returned as -1.

So, implementations are "discouraged" from yielding {0,nil}, but that doesn't mean they won't.

I think your code handles all the relevant cases; if an EOF comes in along with the successful read, we'll end up with {32, EOF}, which is a successful read of 32 bytes, and that seems correct.

I sort of wish the initial Read spec had been slightly more dogmatic about what is permitted.

Re-reading it, your code is also clearer as to intent; it's not retrying exactly 32 times, it's running until it gets an error or gets 32 bytes, which could take more than 32 loops, depending on the reader. It won't handle a TimeoutReader, but then, I'm not sure I've ever seen production code which would, unless it was explicitly using timeouts and expecting to encounter and handle those errors, which most code isn't.

[gopherbot]

Change https://golang.org/cl/143677 mentions this issue: doc: tweak example in Effective Go

[RalphCorderoy]

The CL's comment mentions endless conversation around the subtleties of Read semantics. That was caused by lack of awareness that the (int, error) return values are orthogonal and (0, nil) is a no-op. I expect that's common amongst Go programmers. The CL promotes that misunderstanding by giving a prominent example of Read reading zero bytes being merged with an error. It could have used the opportunity to widen the knowledge of the no-op return.

[seebs]

That might be a good thing to discuss in Effective Go, but I don't think it's a good thing to try to explain in the slice example.