Conversation
Fix bug introduced in pires#116 where io.MultiReader only reads from buffered reader. Move buffer reader management to readHeader(). Remove Conn.bufReader, and make Conn.reader nil until readHeader() is called.
| buf := make([]byte, br.Buffered()) | ||
| if _, err := br.Read(buf); err != nil { | ||
| return err // this should never as we read buffered data | ||
| } |
There was a problem hiding this comment.
This is not correct as Read may read less than Buffered() so the proper way is to use io.ReadFull but then I am not sure it won't trigger another read from p.conn (i.e. I am not sure br.Buffered() will be 0 after that).
That is why I used TeeReader in #119 to copy data read from connection into the bytes.NewBuffer
There was a problem hiding this comment.
This is not correct as Read may read less than Buffered() so the proper way is to use io.ReadFull but then I am not sure it won't trigger another read from p.conn (i.e. I am not sure br.Buffered() will be 0 after that).
I think we can use Peek here to avoid allocation like:
if br.Buffered() > 0 {
b, _ := br.Peek(br.Buffered())
p.reader = io.MultiReader(bytes.NewReader(b), p.conn)
} else {
p.reader = p.conn
}Its not super documented but I think Peek's design is to avoid underlying Read if n <= Buffered()
There was a problem hiding this comment.
Its not super documented but I think Peek's design is to avoid underlying Read if n <= Buffered()
Its not guaranteed unfortunately, see golang/go#63548 (comment)
There was a problem hiding this comment.
@AlexanderYastrebov bufio.Reader.Read will never read here.
Call to read will jump directly here, this is because:
n != 0- len(p)b.r != b.w- there is something buffered
Also, note definition of Buffered().
There was a problem hiding this comment.
The question is about correctness.
There is simply no documented way to obtain buffered data from bufio.Reader and the current implementation can change in the future and this will break unexpectedly. See discussion golang/go#63548 for more details.
To read exactly len(p) bytes, use io.ReadFull(b, p).
but that does not guarantee that reader does not buffer next chunk.
Therefore I think the right way is to capture everything that was read from the connection like I did in #119
There was a problem hiding this comment.
The probability they would change that Read behavior is close to 0.
But we sure can add a test that would guarantee that the Read does not touch the underlying reader.
At this point both branches are very similar, except for the fact that this one is easier to read and uses less resources.
I think you would agree this code is not very simple.
bb := bytes.NewBuffer(make([]byte, 0, bufSize))
br := bufio.NewReaderSize(io.TeeReader(p.conn, bb), bufSize)
Fix bug introduced in #116 where
io.MultiReaderonly reads from buffered reader.Move buffer reader management to
readHeader().Remove
Conn.bufReader, and makeConn.readernil untilreadHeader()is called.