The example code is pretty much asking for panic:
it is equivalent to this:
var wr io.Writer
wr.Write([]byte("asdf"))

This is working as intended.

@minux What? Are you kidding me? wr is not nil. Calls to it should not panic. This can be fixed with a nil check in Flush.

How does a caller check for this to avoid panic?

wr := bufio.NewWriter(&buf)
wr.Reset(nil)
wr.Write([]byte("asdf"))
wr.Flush()

The 2nd line set the underlying writer of wr to nil, and
the fourth line ask bufio.Writer to write to a nil writer,
is that any different from my example?

var wr io.Writer
wr.Write([]byte("asdf"))

@minux Please read into the history covered in moby/moby#10228 and the panic at https://gist.github.com/stevvooe/12e63e5f902b5ba7ec01. The reality is much more subtle but the behavior is broken.

I conceded above that the use case is not valid but the panic is not expected.

I fail to see why the panic is not expected. Why Reset(nil)?
Why Write and Flush after reseting the underlying writer to
nil?

Adding the check to the Flush method is just hiding bugs in
the client code. As using an invalid io.Writer is a programming
bug, panic is the correct way.

@minux "As using an invalid io.Writer is a programming
bug, panic is the correct way"

Flushing on a closed Writer is a programming bug too, and yet Go returns an error instead if panicking in this case. Sounds inconsistent to me.

Valid use cases of panic is an excellent question to ask on the
golang-nuts mailing list, but not here. I'm sure gophers there will
explain the idiomatic way of using panics much better than me.
Thanks.

We're not going to make every standard package function to
check for nil interfaces before calling method on them.

@stevvooe bufio.Writer.Reset(w) is setting the underlying writer to w; if Reset is called with nil, that writer is nil. Arguably that is a programmer error, or shall I say "invalid use" of the library. In your case you see a panic in Flush, but any function that eventually writes to the underlying nil writer will panic (e.g.: http://play.golang.org/p/P_f8BbNvCa).

I'd agree with @minux that it's not the job of the library to check for nil each time in this case. And even if one would check, that would lead to a difficult to use library because in the case of bufio, the panics (or errors, rather, if we check) would come at somewhat "arbitrary times". For instance in the example I gave above, all is fine if the buffer is large enough and never flushed, and the programming error is thus hidden as @minux @Aluded to.

That said, I think an argument could be made that the bufio factory functions (NewReader, NewScanner, NewWriter, ... ) but also functions modifying the underlying data structure (Reset, etc.) should check their arguments for validity and report an error otherwise.

However, this is not how this library was written. In case of the factory functions, one could argue that they should return a nil Reader/Writer/what-have-you if the incoming arguments are invalid. But that again puts the burden on each client, even clients that don't do anything wrong, because they cannot know if something else might cause a nil return.

To make a long story short: this package is written with the assumption that the supplied readers and writers are valid: Calling bufio.NewWriter with a nil underlying writer simply makes no sense.

Thus I must agree with @minux that this is working as intended. If one wanted to be very explicit, the documentation could say that the supplied arguments must be non-nil. At best this is a documentation issue.

@griesemer Should the documentation be updated to indicate that once a nil argument is passed, the underlying bufio.Writer is no longer valid and could panic at anytime?

Because in majority of cases passing nil interface is invalid, I don't
think we need
to make that explicit. The document only needs to point out when an
interface
argument can be legally nil and what will happen if it's nil.

Similarly, if a function takes a pointer, it's invalid to pass a nil
pointer unless its
document explicitly allows that and generally it will also document what
will happen
in that case.

I'd like to add that we can't check for nil in (*bufio.Writer).Reset,
because
that will be an incompatible API change.

It's legal to use wr.Reset(nil) to mark (poison) a Writer as not be used
again so that subsequent uses will very likely generate a panic. This is
another reason that we can't make Flush check for nil writers, the panic
might be intended.

@minux It would preferable that all calls to a nil writer panic, rather than the inconsistent behavior encountered now. Part of the complexity of tracking this down was the fact that the bufio.Writer only panicked at certain times making it hard to expose the underlying issue with writer lifecycle management.

I'll note that wr.Reset may have never actually been called in the original triggering code. This was encountered during a complex interaction where an http.ResponseWriter ends up as part of the reader passed to http.NewRequest. This report was provided with a minimal test case to trigger the same panic, which I thought was a related but different problem.

Thank you for looking at this tonight.

Unfortunately, in general we can't check if a method call will
panic unless we actually invoke it.

However, bufio.Writer is there to reduce the number of calls to
the underlying writer, so reliably checking problem during write
will defeat the whole purpose of buffered IO.

As an extreme example, if I pass a typed nil interface (i.e. if T
implements io.Writer, pass (*T)(nil) as the writer), we can't simply
check if the interface itself is nil, and calling the Write method
might still panic (this depending on how T.Write is implemented).
Thus he "random" panic problem you describe will still happen
even if we add the check for nil interfaces. (And this problem
is even harder to debug than that caused by nil interfaces.)

I have to agree that this is all working as intended. @minux and @griesemer mentioned all the most important points: Reset(nil) must remain as supported to allow people to free a Writer without freeing the buffer of a pooled bufio.Writer. It's the callers mistake to use that bufio.Writer before Resetting it back to something valid.

I took a look at the stack trace you mentioned,
https://gist.github.com/stevvooe/12e63e5f902b5ba7ec01

I'm sure it's due to a nil writer. FYI, the analysis goes as follows:

1. line 530 of bufio/bufio.go is:
   n, err := b.wr.Write(b.buf[0:b.n])
   this line needs the following memory accesses:
   (1) load b.wr and b.n, this requires b to be valid Writer pointer
   (2) load b.buf, slice it, this requires b to valid Writer pointer, but it
   doesn't need b.buf to contain valid pointer.
   (3) load the function pointer from b.wr.
   (4) call the function indirectly

b is not nil, as we can see from the arguments dump, b == 0xc208208900.
so (1) and (2) are not the cause.

(4) might cause problem but note that pc=0x78154a is not zero, so the
only possibility is step 3 (and generally if the pc is wrong in (4), it is
almost
certain a memory corruption bug.)

1. Note that addr=0x20 in the 2nd line. This is address that is being
   accessed. 0x20 is precisely the offset of first function pointer in an itab
   (itab makes up the first half of a non-empty interface).

As the interface in question is Writer, which only contains one method,
the address of Write should be offset 0x20. at this is consistent with our
analysis.

so the cause is itab == nil, this is only possible if the whole interface
is nil.

Hope this helps when you diagnose problems like this in the future.

@minux @griesemer @bradfitz Thank you all for the careful responses.

I do see your point on the basis that calling (*bufio.Writer).Reset(nil) should result in a poised writer. On principle, this makes sense.

However, having the entire process panic when http.Client holds onto an io.Reader that may no longer be valid seems problematic. If you look at the tip of the trace again, the call to (*bufio.Writer).Reset(nil) is a part of the buffer pool implementation in net/http because its the buffer on *http.response that is actually causing the panic:

panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x20 pc=0x78154a]

goroutine 92 [running]:
bufio.(*Writer).flush(0xc208208900, 0x0, 0x0)
    /usr/local/go/src/bufio/bufio.go:530 +0xda
bufio.(*Writer).Flush(0xc208208900, 0x0, 0x0)
    /usr/local/go/src/bufio/bufio.go:519 +0x3a
net/http.(*response).Flush(0xc2085bfae0)
    /usr/local/go/src/net/http/server.go:1047 +0x4c

(from https://gist.github.com/stevvooe/12e63e5f902b5ba7ec01)

I'll drop this issue for now and see about a coming up with a reproduction that is simpler than the docker code base. For now, we have a fix that isolates the client's Close call from response's possibly closed writer.