code review 6285050: fmt, encoding/gob: fix misuse of Read

fmt, encoding/gob: fix misuse of Read
reader.Read() can return both 0,nil and len(buf),err.
To be safe, we use io.ReadFull instead of doing reader.Read directly.

Fixes issue 3472.

[minux1, 2012-12-13 14:18:57]

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

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2012-12-13 16:05:37]

https://codereview.appspot.com/6285050/diff/11002/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (right):

https://codereview.appspot.com/6285050/diff/11002/src/pkg/fmt/scan.go#newcode343
src/pkg/fmt/scan.go:343: } else if n == 0 && err == nil {
doe this ever happen?  The way I've always read io.Reader's contract,

"If some data is available but not len(p) bytes, Read conventionally returns
what is available instead of waiting for more."

... is that you block until at least 1 byte.

I know it doesn't say that, though.

It feels like this file doesn't need changing.  Or if so, why not ReadFull?

[rsc, 2012-12-13 16:15:00]

https://codereview.appspot.com/6285050/diff/11002/src/pkg/encoding/gob/decode.go
File src/pkg/encoding/gob/decode.go (right):

https://codereview.appspot.com/6285050/diff/11002/src/pkg/encoding/gob/decode...
src/pkg/encoding/gob/decode.go:66: if n < 1 {
You can just use 

if n == 0 {
    return
}

The only goal is to avoid bailing when we get n==1, err != nil.

https://codereview.appspot.com/6285050/diff/11002/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (right):

https://codereview.appspot.com/6285050/diff/11002/src/pkg/fmt/scan.go#newcode341
src/pkg/fmt/scan.go:341: if n == 1 {
Same here: if n == 1 { err = nil }
is all you need.

[minux1, 2012-12-13 16:16:47]

On Fri, Dec 14, 2012 at 12:05 AM, <bradfitz@golang.org> wrote:

> https://codereview.appspot.**com/6285050/diff/11002/src/**
>
pkg/fmt/scan.go#newcode343<https://codereview.appspot.com/6285050/diff/11002/src/pkg/fmt/scan.go#newcode343>
> src/pkg/fmt/scan.go:343: } else if n == 0 && err == nil {
> doe this ever happen?  The way I've always read io.Reader's contract,
>
> "If some data is available but not len(p) bytes, Read conventionally
> returns what is available instead of waiting for more."
>
> ... is that you block until at least 1 byte.
>
> I know it doesn't say that, though.
>
I originally though so too, but as Russ commented in
http://code.google.com/p/go/issues/detail?id=3472#c1
Read can return 0, nil (which I think most of Read users are not able to
deal with), or
len(buf), err

> It feels like this file doesn't need changing.  Or if so, why not
> ReadFull?
>
ReadFull has its own problems dealing with "naughty" io.Readers.

I think the essential question is: what to do with a Read that returns 0,
nil?
Should we report an error or try again?

[minux1, 2012-12-13 16:25:58]

PTAL.

On Fri, Dec 14, 2012 at 12:15 AM, <rsc@golang.org> wrote:

> https://codereview.appspot.**com/6285050/diff/11002/src/**
>
pkg/encoding/gob/decode.go#**newcode66<https://codereview.appspot.com/6285050/diff/11002/src/pkg/encoding/gob/decode.go#newcode66>
> src/pkg/encoding/gob/decode.**go:66: if n < 1 {
> You can just use
>
> if n == 0 {
>     return
> }
>
> The only goal is to avoid bailing when we get n==1, err != nil.
>
>
<https://codereview.appspot.com/6285050/diff/11002/src/pkg/fmt/scan.go#newcode341>
>


> https://codereview.appspot.**com/6285050/diff/11002/src/**
>
pkg/fmt/scan.go#newcode341<https://codereview.appspot.com/6285050/diff/11002/src/pkg/fmt/scan.go#newcode341>
> src/pkg/fmt/scan.go:341: if n == 1 {
> Same here: if n == 1 { err = nil }
> is all you need.
>
So we choose to not handle the 0, nil case?
If we can't find a way to deal with that case, can we say that Read
shouldn't return 0, nil
as imo it will break most of the code.

[minux1, 2012-12-13 16:37:48]

On Fri, Dec 14, 2012 at 12:16 AM, minux <minux.ma@gmail.com> wrote:

> ReadFull has its own problems dealing with "naughty" io.Readers.

consider a reader r which returns len(buf), err
but err != io.EOF

should io.ReadFull(r, buf) return an error or not?

I tried to fix ReadFull by not returning the error if Read already
fills the buf, however, at least one test failed.

--- FAIL: TestDecoderIssue3577 (5.00 seconds)
base64_test.go:275:     timeout; Decoder blocked without returning an error
FAIL

maybe we should note for ReadFull that users should still look at n instead
of
err first, but unfortunately, a lot of code make the assumption that if
ReadFull
returns an error, the buf is not completely filled.

[rsc, 2012-12-13 16:49:22]

I am not worried about Read returning 0, nil in these cases. That's
allowed but if you are doing Fscanf on a UDP socket you kind of
deserve whatever you get.

Whether we should change ReadAtLeast and ReadFull to clear err when
returning a full buffer is something I'd still like to think more
about. I created 4544 for that.

Russ

[bradfitz, 2012-12-13 20:25:29]

On Thu, Dec 13, 2012 at 8:49 AM, Russ Cox <rsc@golang.org> wrote:

> I am not worried about Read returning 0, nil in these cases. That's
> allowed but if you are doing Fscanf on a UDP socket you kind of
> deserve whatever you get.
>

What does a UDP socket have to do with this?

Either Read can return (0, nil) and we have to deal with it, or Read can't
return (0, nil) and we don't.

Whether it's a UDP socket or package foo or package bar doesn't matter.

Even if pkg foo vs pkg bar were an issue, both fmt and encoding/gob can
work with many types of Readers.

[rsc, 2012-12-13 20:30:50]

On Thu, Dec 13, 2012 at 3:25 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> What does a UDP socket have to do with this?

The readers that can return 0, nil are ones modeling a packet-based
input stream, such as UDP datagrams.

Russ

[minux1, 2012-12-13 20:31:25]

On Fri, Dec 14, 2012 at 4:25 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> On Thu, Dec 13, 2012 at 8:49 AM, Russ Cox <rsc@golang.org> wrote:
>
>> I am not worried about Read returning 0, nil in these cases. That's
>> allowed but if you are doing Fscanf on a UDP socket you kind of
>> deserve whatever you get.
>>
>
> What does a UDP socket have to do with this?
>
i guess it's because it's possible to get a 0-byte (payload) UDP packet, so
it's ok for (*net.UDPConn).Read
to return 0, nil for that case.

>
> Either Read can return (0, nil) and we have to deal with it, or Read can't
> return (0, nil) and we don't.
>
I think the situation is: Read can return (0, nil) for the UDP case, but we
really can't do much about it.

[bradfitz, 2012-12-13 20:36:14]

On Thu, Dec 13, 2012 at 12:30 PM, Russ Cox <rsc@golang.org> wrote:

> On Thu, Dec 13, 2012 at 3:25 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > What does a UDP socket have to do with this?
>
> The readers that can return 0, nil are ones modeling a packet-based
> input stream, such as UDP datagrams.


Wow, crazy.  Not the answer I was expecting.

I thought io.Reader was a byte stream interface, not a message-stream
interface.

If somebody cares about UDP boundaries, they can just use package net and
UDPConn directly, can't they?

Can we update the docs on io.Reader to either a) make this explicit about
UDP sockets, or b) outright ban (0, nil) from io.Readers and fix
UDPConn.Read?  (I would prefer b)

[rsc, 2012-12-13 20:40:24]

I need to think more about all this but I don't have any uninterrupted
chunks of time today. Issue 4544 reminds me to do that later. :-)

[minux1, 2012-12-13 20:47:53]

On Fri, Dec 14, 2012 at 4:36 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote:
>
> On Thu, Dec 13, 2012 at 12:30 PM, Russ Cox <rsc@golang.org> wrote:
>
>> On Thu, Dec 13, 2012 at 3:25 PM, Brad Fitzpatrick <bradfitz@golang.org>
>> wrote:
>> > What does a UDP socket have to do with this?
>>
>> The readers that can return 0, nil are ones modeling a packet-based
>> input stream, such as UDP datagrams.
>
> I thought io.Reader was a byte stream interface, not a message-stream
> interface.
>
> If somebody cares about UDP boundaries, they can just use package net and
> UDPConn directly, can't they?
>
> Can we update the docs on io.Reader to either a) make this explicit about
> UDP sockets, or b) outright ban (0, nil) from io.Readers and fix
> UDPConn.Read?  (I would prefer b)

i think option b will break (*net.UDPConn).Read's established Go 1
semantics.
Although I agree with you about the implied byte stream style interface for
io.Reader.

no matter which solution we choose, i think we need more extensive
documentation
on io.Reader, and the official and correct way to deal with its returned
values.

the docs for io.Writer is much clearer about error returns than io.Reader,
i wonder
where does the extra complexities for io.Reader come from?

[ality, 2012-12-14 00:24:36]

Brad Fitzpatrick <bradfitz@golang.org> once said:
> b) outright ban (0, nil) from io.Readers

It's not just UDP. The preservation of message boundaries can be very
useful if you're not stuck with Unix. On Plan 9 there are a few programs
that take advantage of zero length writes and I want to be able to
interface with them.

Please don't add this artificial restriction to io.Readers.

  Anthony

[bradfitz, 2012-12-14 04:47:07]

On Thu, Dec 13, 2012 at 4:24 PM, Anthony Martin <ality@pbrane.org> wrote:

> Brad Fitzpatrick <bradfitz@golang.org> once said:
> > b) outright ban (0, nil) from io.Readers
>
> It's not just UDP. The preservation of message boundaries can be very
> useful if you're not stuck with Unix. On Plan 9 there are a few programs
> that take advantage of zero length writes and I want to be able to
> interface with them.
>
> Please don't add this artificial restriction to io.Readers.
>

This isn't a discussion about Plan 9 semantics.  This is a discussion about
io.Reader semantics.

io.Reader does not document any message boundary guarantees today.  Any you
happen to get right now are a side-effect of the Plan 9 port's
implementation, and not language guarantees.

[minux1, 2012-12-14 04:56:36]

On Fri, Dec 14, 2012 at 8:24 AM, Anthony Martin <ality@pbrane.org> wrote:

> Brad Fitzpatrick <bradfitz@golang.org> once said:
> > b) outright ban (0, nil) from io.Readers
>
> It's not just UDP. The preservation of message boundaries can be very
> useful if you're not stuck with Unix. On Plan 9 there are a few programs
> that take advantage of zero length writes and I want to be able to
> interface with them.
>
Could you please elaborate a bit?

>
> Please don't add this artificial restriction to io.Readers.
>
I think the problem is: allowing (0, nil) return will complicate all users
of
io.Readers, while few programs actually require this capability.

Allowing it means you are on your own if you're using this capability
because
even the standard library could not handle it (i think correctly handle it
means
we can't use Read() directly, and must resort to something like
io.ReadAtLeast
or io.ReadFull)

[rsc, 2012-12-17 16:17:23]

Please change this code to use io.ReadFull. Then at least we can get
this CL behind us. The other issue remains open.

Thanks.
Russ

[minux1, 2012-12-17 16:33:00]

On 2012/12/17 16:17:23, rsc wrote:
> Please change this code to use io.ReadFull. Then at least we can get
> this CL behind us. The other issue remains open.
Sure. PTAL.

[rsc, 2012-12-17 17:01:32]

LGTM

Thanks.

[minux1, 2012-12-17 17:27:04]

*** Submitted as https://code.google.com/p/go/source/detail?r=7ea3674ce4b5 ***

fmt, encoding/gob: fix misuse of Read
reader.Read() can return both 0,nil and len(buf),err.
To be safe, we use io.ReadFull instead of doing reader.Read directly.

Fixes issue 3472.

R=bradfitz, rsc, ality
CC=golang-dev
https://codereview.appspot.com/6285050