Should Conn.Read(b) respect io.Reader contract strictly or not?

[pires]

Currently, Conn.Read(p []byte) returns any data available. Consumers are expected to continue calling Read(p) until they either get the data they need or an error is returned. This follows idiomatic Go, namely the io.Reader.Read(b) documented behavior:

Read reads up to len(p) bytes into p. It returns the number of bytes read (0 <= n <= len(p)) and any error encountered.
(...)
If some data is available but not len(p) bytes, Read conventionally returns what is available instead of waiting for more.

However, some users of this library have different expectations. As per @clementnuss in #148 (comment):

in using this library though I'm expecting to transparently hit the underlying conn. therefore, when making a call to pConn.Read() with a 4KiB buffer, I am not at all expecting that the library will only return 210 bytes.
the alternatives are not compelling either: io.ReadAll would be reading too many bytes and io.ReadFull would return ErrUnexpectedEOF if there's an EOF before my buffer is filled.

if I want to mimic the "transparent" behavior (i.e. providing a 4KiB buffer and receiving 4KiB if those are available on the OS buffers), I'll have to make double Read calls as a user, to first drain the bufferedReader and then reach the actual Conn.
This workaround is both impractical and counter-intuitive, as I would expect from a library that transparently pipes an underlying conn to transparently give me access to the underlying conn, but at least with your fix I would actually reach the underlying Conn and this solution would work (with the previous version that would've been impossible)

So the question is should Conn.Read(b) respect io.Reader contract strictly or not?

Refs #142 #148

Potentially interested contributors: @bollenberger @mmatczuk @emersion @clementnuss @AlexanderYastrebov

[pires]

I understand the frustration with the short read. However, net.Conn does not support a non-blocking 'peek' to check for OS-level buffers. If we attempt to read from the underlying connection to fill your desired buffer and the OS buffer happens to be empty, Read(p) will block indefinitely, causing a deadlock.

The behavior implemented in the library should match the Go standard library's io.MultiReader - and that's why we wrongly used that facility and are trying to fix in the likes of #148 (fixed).

If you require filled buffers, I recommend ~wrapping the connection in your own bufio.Reader using io.ReadFull() or

buf := make([]byte, 16384)
for totalRead < payloadSize {
    n, err := conn.Read(buf[totalRead:])
    if err != nil {
        break // or handle errors but io.EOF
    }
    totalRead += n
}

[pires]

I had to update my answer as I had yet again deviated from my initial understanding of the issue.

[clementnuss]

doing so will however block if say we have emptied the OS recv buffer but keep calling conn.Read() to try and fill 16384 bytes. the OS will likely have one TCP MSS available, so to get that data I shouldn't use io.ReadFull() as it would block, but I would do a rather ugly "double-read" in the caller code.

we will also typically not receive an EOF at this stage, as the other end likely hasn't closed the connection, which is why ReadFull will end up blocking

[emersion]

• I would find it surprising if a Conn.Read call results in multiple Read calls under-the-hood.
• I would find it surprising if bufio.Reader immediately has data available but blocks for more data instead of returning it.
• bufio.Reader never guarantees that it will return full 4KiB chunks. bufio.Reader only refills its internal buffer when it needs more data. If the writer is slow, bufio.Reader may return smaller chunks.
• When an io.Reader is wrapped by another type, it's completely fine for the wrapper to fragment Read calls into smaller chunks.

That said, I wonder if we could avoid creating a a bufio.Reader if the io.Reader passed in is already a bufio.Reader. The stdlib does this in a few places, to avoid accumulating multiple layers of bufio.Reader. Since we're accepting net.Conn instead of io.Reader, this sounds a bit trickier to do in go-proxyproto, though.

[clementnuss]

what about the following: bufReader is being sized dynamically, and is set to max(256,len(b)), where b is the buffer passed by the user in conn.Read(b) ?

that way if someone just wants to parse the proxy protocol header they can use conn.ProxyHeader() and the default (256) bufReader buffer size will be used.

and if the caller provides a larger buffer size (because they likely expect more data than just 256 bytes to be delivered), then that size is used as the bufReader size.

that's what I implemented in #162 😄

[pires]

I am considering an alternative approach where you have per-conn control of the internal buffer size.

func WithBufferSize(length int) func(*Conn) {
	return func(c *Conn) {
		if length > 0 {
			// bufReaderCustomSize is *int
			l := length
			c.bufReaderCustomSize = &l
		}
	}
}

(...)

myconn := NewConn(conn, WithBufferSize(4096))
_, _ = myconn.Read(p)

WDYT?

[clementnuss]

why not, that would be explicit. I can modify my PR in that direction if you want

[pires]

Please, test and review #163 🙏🏻