Using interruptible bidirectional copying to keep idle connections fr… #4707
Conversation
| bufin := pool.Get() | ||
| bufout := pool.Get() | ||
| defer pool.Put(bufin) | ||
| defer pool.Put(bufout) |
There was a problem hiding this comment.
Note - using a buffer pool is not necessary to fix this bug, but it's a useful optimization anyway.
…om piling up, closes getlantern/lantern#4686
|
|
||
| // Then start copying from proxy to client. | ||
| _, readErr := io.Copy(clientConn, connOut) | ||
| writeErr := <-writeErrCh |
There was a problem hiding this comment.
Where in the previous code are connections leaking @oxtoacart?
There was a problem hiding this comment.
There is no leak. It's just that we don't close the connections until both the reading and the writing are done. That meant that we were relying on the idle timeout to close the connections, which would stop the reader and writer.
There was a problem hiding this comment.
Wait sorry but what's bad about not closing the connection until both reading and writing are done? Why would we want to close them before they're done or before the idle timeout @oxtoacart?
There was a problem hiding this comment.
Here's the scenario:
- Client connects
- Proxy connects to server
- Proxy starts piping both ways
- Client disconnects
- Reading from the client will stop at this point due to an EOF, but writing to the client doesn't stop, so we keep both connections open
The simplistic way to do this is to just close both connections as soon as either io.Copy is finished, which is what we used to do, but that sometimes results in us closing a connection while we're still doing an io op on it. The new code in netx sets timeouts on the io ops and interrupts in between ops.
There was a problem hiding this comment.
Gotcha @oxtoacart. OK the code looks fine to me. Merging!
|
Although it looks like one of the netx tests is failing @oxtoacart... |
|
@myleshorton I fixed the test for Linux. |
| nr, er := src.Read(buf) | ||
| if nr > 0 { | ||
| nw, ew := dst.Write(buf[0:nr]) | ||
| if ew != nil && !isTimeout(err) { |
There was a problem hiding this comment.
No. Timeouts are expected because we're intentionally setting a short deadline to give us a chance to unblock and check the stop flag.
There was a problem hiding this comment.
Ah, yes, great catch!
Great catch! |
git-subtree-dir: src/github.com/getlantern/netx git-subtree-split: 427296094076c85817e2bdddcc9694b685d58737
…b.com/getlantern/netx'
Using interruptible bidirectional copying to keep idle connections fr…
…om piling up, closes getlantern/lantern#4686