protocol: limit use of buffered reader

[mmatczuk]

Fix bug introduced in #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.

[coveralls]

coverage: 94.468% (+0.3%) from 94.136%
when pulling 8a8a4d7 on mmatczuk:mmt/avoid_read_buffer
into bac82fd on pires:main.

[AlexanderYastrebov]

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

[AlexanderYastrebov]

BTW err could be EOF

[AlexanderYastrebov]

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()

[AlexanderYastrebov]

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)

[mmatczuk]

@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().

[AlexanderYastrebov]

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.

From https://pkg.go.dev/bufio#Reader.Read

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

[mmatczuk]

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)

[pires]

Can you, please, add such test?

[pires]

ping @mmatczuk 🙏🏻