bufio: preemptive Flush() inside `Write*()` methods #20943

gobwas · 2017-07-07T12:26:11Z

Hello!

While digging the sources of bufio.Writer I had a question:
Inside Writer.ReadFrom() method exists a "preemptive" Flush(). I think this is made to deal with src.Read() deadlines in subsequent calls of ReadFrom – to not spent time on buffer flush before read attempt.

Could it be useful to make the same "preemptive" flushes inside other Writer.Write*() methods?

I mean that in current implementation test like this will fail:

type snoozeWriter struct {
	delay time.Duration
}

func (w snoozeWriter) Write(p []byte) (int, error) {
	time.Sleep(w.delay)
	return len(p), nil
}

type deadlineReader struct {
	deadline time.Time
}

func (r deadlineReader) Read(p []byte) (int, error) {
	if time.Since(r.deadline) > 0 {
		return 0, fmt.Errorf("deadline exceeded")
	}
	return len(p), io.EOF
}

func TestWriterReadFromTimeout(t *testing.T) {
	const size = 10

	bw := bufio.NewWriterSize(snoozeWriter{time.Second}, 10)

	// Fill buffer, without Flush().
	bw.WriteString("0123456789")

	// Emulate some reader with read deadline set.
	r := deadlineReader{time.Now().Add(500 * time.Millisecond)}

	if n, err := bw.ReadFrom(r); n != size || err != nil {
		t.Fatalf("ReadFrom() = (%v, %v); want (10, nil)", n, err)
	}
}

The text was updated successfully, but these errors were encountered:

rsc · 2017-07-11T13:20:31Z

In general the rule for buffering I/O is to buffer as much as possible and therefore to flush as little as possible. Write implementations should check b.Available() > 0 before starting to write; if that's not true, they should call Flush then, before the write operation. The code you found was added to fix #5947, in which ReadFrom did not check for b.Available() > 0 before starting and therefore could be confused by trying to read 0 bytes. In the fix for that issue, Andrew did a kind of belt-and-suspenders fix and checked for a full buffer both before and after the operation. Only the one before is needed. The one after (the one you found) could potentially be removed for Go 1.10.

But I'm inclined to just leave it alone. It's not bothering anyone. At the same time, I don't want to introduce more preemptive flushes - they'll just hide bugs where Flush has been forgotten.

gobwas · 2017-07-11T20:22:02Z

@rsc Thanks for clarification. Yeah, removing the "one after" is looks fine too. By the way, after few days pass, I realise that preemptive flush after Write() with buf.Available() bytes will produce unexpected behaviour – caller probably may want to fill buffer, but not to flush it. So yeah, its ok, thanks.

bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 7, 2017

bradfitz added this to the Go1.10 milestone Jul 7, 2017

rsc closed this as completed Jul 11, 2017

golang locked and limited conversation to collaborators Jul 11, 2018

gopherbot added the FrozenDueToAge label Jul 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bufio: preemptive Flush() inside `Write*()` methods #20943

bufio: preemptive Flush() inside `Write*()` methods #20943

gobwas commented Jul 7, 2017 •

edited

rsc commented Jul 11, 2017

gobwas commented Jul 11, 2017

bufio: preemptive Flush() inside Write*() methods #20943

bufio: preemptive Flush() inside Write*() methods #20943

Comments

gobwas commented Jul 7, 2017 • edited

rsc commented Jul 11, 2017

gobwas commented Jul 11, 2017

bufio: preemptive Flush() inside `Write*()` methods #20943

bufio: preemptive Flush() inside `Write*()` methods #20943

gobwas commented Jul 7, 2017 •

edited