Issue 76400048: code review 76400048: bufio: fix bug that ReadFrom stops before EOF or error (Closed)
Descriptionbufio: fix bug that ReadFrom stops before EOF or error
ReadFrom should not return until it receives a non-nil error
or too many contiguous (0, nil)s from a given reader.
Currently it immediately returns if it receives one (0, nil).
Fixes issue 7611.
Patch Set 1 #Patch Set 2 : diff -r 62052ebe728b https://code.google.com/p/go #Patch Set 3 : diff -r 62052ebe728b https://code.google.com/p/go #Patch Set 4 : diff -r 62052ebe728b https://code.google.com/p/go #Patch Set 5 : diff -r 62052ebe728b https://code.google.com/p/go #Patch Set 6 : diff -r 62052ebe728b https://code.google.com/p/go #Patch Set 7 : diff -r 62052ebe728b https://code.google.com/p/go #Patch Set 8 : diff -r 62052ebe728b https://code.google.com/p/go #Patch Set 9 : diff -r 62052ebe728b https://code.google.com/p/go #
Total comments: 2
MessagesTotal messages: 20
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
This looks like another place for io.ErrNoProgress. Count the number of contiguous (0, nil) Read calls you get. If you see 2 or maybe 3 in a row, then return io.ErrNoProgress. (which is the "your Reader is broken" error)
Sign in to reply to this message.
Is that the right thing to do? The manual of io.ReaderFrom does say that it reads data from a given reader until EOF or error. I read that ReadFrom should not generate an error by itself unless it gets an error from a reader. http://golang.org/pkg/io/#ReaderFrom ReadFrom reads data from r until EOF or error. The return value n is the number of bytes read. Any error except io.EOF encountered during the read is also returned. Returning (0, nil) does not mean the reader is broken, so I'd think two or three consecutive (0, nil) should not also be treated as an error. On 2014/03/24 15:56:37, bradfitz wrote: > This looks like another place for io.ErrNoProgress. > > Count the number of contiguous (0, nil) Read calls you get. If you see 2 or > maybe 3 in a row, then return io.ErrNoProgress. (which is the "your Reader is > broken" error)
Sign in to reply to this message.
On Mon, Mar 24, 2014 at 9:58 AM, <ruiu@google.com> wrote: > Is that the right thing to do? The manual of io.ReaderFrom does say that > it reads data from a given reader until EOF or error. I read that > ReadFrom should not generate an error by itself unless it gets an error > from a reader. > > http://golang.org/pkg/io/#ReaderFrom > ReadFrom reads data from r until EOF or error. The return value n is > the number of bytes read. Any error except io.EOF encountered during the > read is also returned. > > Returning (0, nil) does not mean the reader is broken, so I'd think two > or three consecutive (0, nil) should not also be treated as an error. Broken Readers returning (0, nil) repeatedly was the reason we added io.ErrNoProgress in the first place. It's relatively new, and not all docs mention it or should. It's a rare detail. Copy whatever policy you see in other places using ErrNoProgress. Maybe it's 20 instead of 2 or 3.
Sign in to reply to this message.
OK, if it's the policy, let's return io.ErrNoProgress on contiguous (0, nil). Both in crypto/tls/conn.go and in bufio/scan.go, we use 100 as the threshold to return ErrNoProgress, so I'll use the same number for this as well. On Mon, Mar 24, 2014 at 10:02 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > > > > On Mon, Mar 24, 2014 at 9:58 AM, <ruiu@google.com> wrote: > >> Is that the right thing to do? The manual of io.ReaderFrom does say that >> it reads data from a given reader until EOF or error. I read that >> ReadFrom should not generate an error by itself unless it gets an error >> from a reader. >> >> http://golang.org/pkg/io/#ReaderFrom >> ReadFrom reads data from r until EOF or error. The return value n is >> the number of bytes read. Any error except io.EOF encountered during the >> read is also returned. >> >> Returning (0, nil) does not mean the reader is broken, so I'd think two >> or three consecutive (0, nil) should not also be treated as an error. > > > Broken Readers returning (0, nil) repeatedly was the reason we added > io.ErrNoProgress in the first place. It's relatively new, and not all docs > mention it or should. It's a rare detail. > > Copy whatever policy you see in other places using ErrNoProgress. Maybe > it's 20 instead of 2 or 3. > > >
Sign in to reply to this message.
SGTM On Mon, Mar 24, 2014 at 10:41 AM, Rui Ueyama <ruiu@google.com> wrote: > OK, if it's the policy, let's return io.ErrNoProgress on contiguous (0, > nil). Both in crypto/tls/conn.go and in bufio/scan.go, we use 100 as the > threshold to return ErrNoProgress, so I'll use the same number for this as > well. > > On Mon, Mar 24, 2014 at 10:02 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > >> >> >> >> On Mon, Mar 24, 2014 at 9:58 AM, <ruiu@google.com> wrote: >> >>> Is that the right thing to do? The manual of io.ReaderFrom does say that >>> it reads data from a given reader until EOF or error. I read that >>> ReadFrom should not generate an error by itself unless it gets an error >>> from a reader. >>> >>> http://golang.org/pkg/io/#ReaderFrom >>> ReadFrom reads data from r until EOF or error. The return value n is >>> the number of bytes read. Any error except io.EOF encountered during the >>> read is also returned. >>> >>> Returning (0, nil) does not mean the reader is broken, so I'd think two >>> or three consecutive (0, nil) should not also be treated as an error. >> >> >> Broken Readers returning (0, nil) repeatedly was the reason we added >> io.ErrNoProgress in the first place. It's relatively new, and not all docs >> mention it or should. It's a rare detail. >> >> Copy whatever policy you see in other places using ErrNoProgress. Maybe >> it's 20 instead of 2 or 3. >> >> >> >
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, bradfitz@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
Can't that 100 constant be shared between the different users of ErrNoProgress in bufio? On Mar 24, 2014 10:57 AM, <ruiu@google.com> wrote: > Hello golang-codereviews@googlegroups.com, bradfitz@golang.org (cc: > golang-codereviews@googlegroups.com), > > Please take another look. > > > https://codereview.appspot.com/76400048/ > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. >
Sign in to reply to this message.
Done. PTAL. On Mon, Mar 24, 2014 at 10:58 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > Can't that 100 constant be shared between the different users of > ErrNoProgress in bufio? > On Mar 24, 2014 10:57 AM, <ruiu@google.com> wrote: > >> Hello golang-codereviews@googlegroups.com, bradfitz@golang.org (cc: >> golang-codereviews@googlegroups.com), >> >> Please take another look. >> >> >> https://codereview.appspot.com/76400048/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "golang-codereviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to golang-codereviews+unsubscribe@googlegroups.com. >> For more options, visit https://groups.google.com/d/optout. >> >
Sign in to reply to this message.
BTW, we may also want to modify text/scanner/scanner.go as Scanner.next loops forever if upstream reader returns (0, nil). I'd think we want it to return ErrNoProgress. I don't have time to work on that at this moment, but will do. On Mon, Mar 24, 2014 at 11:03 AM, Rui Ueyama <ruiu@google.com> wrote: > Done. PTAL. > > > On Mon, Mar 24, 2014 at 10:58 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > >> Can't that 100 constant be shared between the different users of >> ErrNoProgress in bufio? >> On Mar 24, 2014 10:57 AM, <ruiu@google.com> wrote: >> >>> Hello golang-codereviews@googlegroups.com, bradfitz@golang.org (cc: >>> golang-codereviews@googlegroups.com), >>> >>> Please take another look. >>> >>> >>> https://codereview.appspot.com/76400048/ >>> >>> -- >>> You received this message because you are subscribed to the Google >>> Groups "golang-codereviews" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to golang-codereviews+unsubscribe@googlegroups.com. >>> For more options, visit https://groups.google.com/d/optout. >>> >> >
Sign in to reply to this message.
Low priority anyway since approximately zero people use text/scanner. On Mar 24, 2014 11:10 AM, "Rui Ueyama" <ruiu@google.com> wrote: > BTW, we may also want to modify text/scanner/scanner.go as Scanner.next > loops forever if upstream reader returns (0, nil). I'd think we want it to > return ErrNoProgress. I don't have time to work on that at this moment, but > will do. > > > On Mon, Mar 24, 2014 at 11:03 AM, Rui Ueyama <ruiu@google.com> wrote: > >> Done. PTAL. >> >> >> On Mon, Mar 24, 2014 at 10:58 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: >> >>> Can't that 100 constant be shared between the different users of >>> ErrNoProgress in bufio? >>> On Mar 24, 2014 10:57 AM, <ruiu@google.com> wrote: >>> >>>> Hello golang-codereviews@googlegroups.com, bradfitz@golang.org (cc: >>>> golang-codereviews@googlegroups.com), >>>> >>>> Please take another look. >>>> >>>> >>>> https://codereview.appspot.com/76400048/ >>>> >>>> -- >>>> You received this message because you are subscribed to the Google >>>> Groups "golang-codereviews" group. >>>> To unsubscribe from this group and stop receiving emails from it, send >>>> an email to golang-codereviews+unsubscribe@googlegroups.com. >>>> For more options, visit https://groups.google.com/d/optout. >>>> >>> >> >
Sign in to reply to this message.
I agree. I just wanted to mention that there are other places for ErrNoProgress and didn't want it would get lost. On Mon, Mar 24, 2014 at 11:18 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > Low priority anyway since approximately zero people use text/scanner. > On Mar 24, 2014 11:10 AM, "Rui Ueyama" <ruiu@google.com> wrote: > >> BTW, we may also want to modify text/scanner/scanner.go as Scanner.next >> loops forever if upstream reader returns (0, nil). I'd think we want it to >> return ErrNoProgress. I don't have time to work on that at this moment, but >> will do. >> >> >> On Mon, Mar 24, 2014 at 11:03 AM, Rui Ueyama <ruiu@google.com> wrote: >> >>> Done. PTAL. >>> >>> >>> On Mon, Mar 24, 2014 at 10:58 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: >>> >>>> Can't that 100 constant be shared between the different users of >>>> ErrNoProgress in bufio? >>>> On Mar 24, 2014 10:57 AM, <ruiu@google.com> wrote: >>>> >>>>> Hello golang-codereviews@googlegroups.com, bradfitz@golang.org (cc: >>>>> golang-codereviews@googlegroups.com), >>>>> >>>>> Please take another look. >>>>> >>>>> >>>>> https://codereview.appspot.com/76400048/ >>>>> >>>>> -- >>>>> You received this message because you are subscribed to the Google >>>>> Groups "golang-codereviews" group. >>>>> To unsubscribe from this group and stop receiving emails from it, send >>>>> an email to golang-codereviews+unsubscribe@googlegroups.com. >>>>> For more options, visit https://groups.google.com/d/optout. >>>>> >>>> >>> >>
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=2e2908226d52 *** bufio: fix bug that ReadFrom stops before EOF or error ReadFrom should not return until it receives a non-nil error or too many contiguous (0, nil)s from a given reader. Currently it immediately returns if it receives one (0, nil). Fixes issue 7611. LGTM=bradfitz R=golang-codereviews, bradfitz CC=golang-codereviews https://codereview.appspot.com/76400048 Committer: Brad Fitzpatrick <bradfitz@golang.org>
Sign in to reply to this message.
Message was sent while issue was closed.
This CL appears to have broken the windows-386-ec2 builder. See http://build.golang.org/log/861ff0e165467bbe2f0b602e2f6e586eae231f04
Sign in to reply to this message.
Message was sent while issue was closed.
Would be good to make the max attempts configurable include infinite, even though this may cause cpu burn https://codereview.appspot.com/76400048/diff/140001/src/pkg/bufio/bufio.go File src/pkg/bufio/bufio.go (right): https://codereview.appspot.com/76400048/diff/140001/src/pkg/bufio/bufio.go#ne... src/pkg/bufio/bufio.go:41: const maxConsecutiveEmptyReads = 100 While a default for this is nice having it as a public variable on Reader so it can be tuned to the use case would be ideal
Sign in to reply to this message.
Message was sent while issue was closed.
https://codereview.appspot.com/76400048/diff/140001/src/pkg/bufio/bufio.go File src/pkg/bufio/bufio.go (right): https://codereview.appspot.com/76400048/diff/140001/src/pkg/bufio/bufio.go#ne... src/pkg/bufio/bufio.go:41: const maxConsecutiveEmptyReads = 100 On 2014/06/06 14:38:08, smh wrote: > While a default for this is nice having it as a public variable on Reader so it > can be tuned to the use case would be ideal Reader doesn't currently have anything public. That'd be a notable change. Also, Go eschews rare knobs that add to cognitive overhead for people reading the docs. This is good as-is.
Sign in to reply to this message.
Message was sent while issue was closed.
The docs need to explicitly mention this behaviour as otherwise its going to catch people out who aren't expecting this this arguably none standard behaviour.
Sign in to reply to this message.
io.Reader already mentions not to return (0, nil) On Jun 6, 2014 9:02 AM, <steven.hartland@multiplay.co.uk> wrote: > The docs need to explicitly mention this behaviour as otherwise its > going to catch people out who aren't expecting this this arguably none > standard behaviour. > > https://codereview.appspot.com/76400048/ >
Sign in to reply to this message.
Message was sent while issue was closed.
On 2014/06/06 16:11:59, bradfitz wrote: > io.Reader already mentions not to return (0, nil) It mentions its not recommended it doesn't say it will break other core modules like bufio after an a seemingly arbitrary limit.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
