Expose Conn.BufReader

[guysv]

To prevent data loss from internal buffer when unwrapping Conn back to the underlying connection, the private buffered reader was made external. Follows-up #51.

[coveralls]

Coverage remained the same at 93.487% when pulling eb536e9 on guysv:master into 9d53ed5 on pires:master.

[coveralls]

Coverage remained the same at 93.487% when pulling eb536e9 on guysv:master into 9d53ed5 on pires:master.

[guysv]

Also, off-topic. #51 mentioned a generic Conn.Raw() to get the underlying connection without casting. where did that go?

[emersion]

Hmm, still not clear to me why it's not possible to just read from proxyproto.Conn.

[emersion]

Also, off-topic. #51 mentioned a generic Conn.Raw() to get the underlying connection without casting. where did that go?

Feel free to submit a PR for it. I don't need that yet, so haven't bothered sending a patch.

[guysv]

Hmm, still not clear to me why it's not possible to just read from proxyproto.Conn.

Reading larger data than whats buffered in bufio.Reader will cause it to read its underlying reader. To drain just the internal buffer, you need to know exactly how much you can read without triggering a refill. That amount you can read is exposed via Reader.Buffered(), which is what I just exposed.

[emersion]

OK, but why do you need to drain the internal bufio reader?

[guysv]

Ah. easy. When you detach the underlying connection from the proxy struct (which is now done using #51), you're going to want that extra data when you repurpose that connection.

In my use case, I'm writing a connection forwarder (open new connection to remote, pipe data between existing (proxy) connection and the new connection). To do that properly, I need to call the underlying TCPConn's CloseRead and CloseWrite

Because those methods are unaware to the fact that some of the data read from the connection is actually buffered somewhere, I'm going to need to:

1. drain the buffer.
2. write the drained data to the outgoing connection.
3. do the usual connection forwarding move (io.Copy(in, out), io.Copy(out, in)).
4. call TCPConn methods to close the connection properly.

If I don't drain the buffer, some application data will be forgotten in that buffer.

[guysv]

By the way, another approach could be setting Conn's net.Conn as the embedded struct. That way you could e.g. pass a Conn to my forwarding routine (I would need to do the casts over there, but that's OK i guess)

[emersion]

Thanks for taking the time to explain everything!

call TCPConn methods to close the connection properly

So in two separate goroutines you want to:

• io.Copy(in, out) and then CloseRead
• io.Copy(out, in) and then CloseWrite

to properly support half-opened connections?

To do that properly, I need to call the underlying TCPConn's CloseRead and CloseWrite

Maybe you can call Read on the proxyproto.Conn and CloseRead on the underlying connection? I guess that makes your internal logic more complicated?

[emersion]

By the way, another approach could be setting Conn's net.Conn as the embedded struct. That way you could e.g. pass a Conn to my forwarding routine (I would need to do the casts over there, but that's OK i guess)

Hm, I don't quite understand this one. What is the "embedded struct"?

[guysv]

yea! that's why I offered

By the way, another approach could be setting Conn's net.Conn as the embedded struct. That way you could e.g. pass a Conn to my forwarding routine (I would need to do the casts over there, but that's OK i guess)

so I could just cast proxyproto.Conn like I would've done with any net.Conn anyway.

I guess that will do fine for my use-case :)

[guysv]

well, in our case we have an embedded interface, but we could pull it off like this

type Conn struct {
-	conn              net.Conn
+	net.Conn
	bufReader         *bufio.Reader
	header            *Header
	once              sync.Once
	ProxyHeaderPolicy Policy
	Validate          Validator
	readErr           error
}

that way proxyproto.Conn "inherits" all of conn's methods (but can override them with implemention of its own for e.g. Read.)

[emersion]

I see, that makes sense.

I believe there are other use-cases that would benefit of exposing the underlying bufio.Reader. For instance, if you want to read a HTTP request from the connection, you may want to use textproto.NewReader. However if you do it like this:

var conn *proxyproto.Conn
tr := textproto.NewReader(bufio.NewReader(conn))

Then the pipeline will contain two buffered readers, one internal to proxyproto.Conn, the other created by the user. This would avoid the overhead:

var conn *proxyproto.Conn
tr := textproto.NewReader(conn.BufReader)

[guysv]

well, buffered reader actually checks if the internal reader is a buffered reader too and reuses it if so, but I see where you're going with that.

[emersion]

well, buffered reader actually checks if the internal reader is a buffered reader too and reuses it if so, but I see where you're going with that.

Yeah, but that won't work here, because *proxyproto.Conn (which is an io.Reader, so can be passed to bufio.NewReader) won't type-assert to a *bufio.Reader.

[guysv]

Well, i've been thinking about it, and at least for my end goal turning net.Conn into the embedded interface will make my code better than draining the however-long bufio.Reader chain I might be facing. May I add that too to the PR for further discussion?

[guysv]

My bad. Excuse me. What I just outlined won't work because i'll need to type-assert to net.Conn before calling my routine, so i'm losing the internal buffer again. I guess I'll have to pass both the buffered reader and the conn to the routine.

[guysv]

OK. After doing some offline evaluation, this PR is irrelevant for my needs, as I could just type-assert to proxyproto.Conn, unwrap to tcpconn with proxyproto.TCPConn() and call the needed CloseRead/Write methods.

I would, however prefer to have that Raw() method we talked about earlier. I'll open another PR for that.

Because we did outline some useful use-cases for this PR's change, feel free to merge it (or close it)

[pires]

hey @guysv! Thank you very much for your contribution, however I'd prefer to not introduce this and have #55 instead 🙇

[emersion]

Have you read #54 (comment)?

[pires]

I re-read it now, thanks for calling it out.

[pires]

Since this is exported, please add a descriptive comment with an example of a use-case. Also, please add a test or two.

[pires]

Gentle ping in case this got lost in doom of github notifications.

[pires]

Actually, I was reading this for the nth time and I want to go back to my initial assessment: this will be closed until it really becomes needed. Right now, it seems like it would be doing something for future users that may never show up and I'd rather not do it, particularly as exposing the buffer this way may also result in dangerous behavior.