ProxyHandler stalls the proxy handshake when auto-read=false #5933

Closed
vkostyukov opened this Issue Oct 21, 2016 · 2 comments

Projects

None yet

2 participants

@vkostyukov
Contributor
vkostyukov commented Oct 21, 2016 edited

We're (Finagle) using the N4 tooling around proxies and just discovered a bug in ProxyHandler. When auto-read is disabled (our default) a connection established with the proxy server goes into a stall state waiting for a first response and never receives that (fires a handshake timeout). This is a common problem and might be reproduced with both HTTP and SOCKS5.

While debugging this we noticed that a response was actually sent by a server, but was never read from the pipeline by a ProxyHandler since it doesn't issue a read-request explicitly and relies on auto-read (or previous handlers to issue the read).

The solution to this bug is pretty straightforward - we have to issue a read request (once for HTTP and twice for SOCKS5) if the auto-read is disabled.

We're working on the PR (with the fix) right now.

@vkostyukov
Contributor

Adding some new details. Turns out we only need to issue one read to trigger the handshake (no matter how many messages it takes to complete for HTTP and SOCKS5) given that ProxyHandler.readComplete() will keep reading for us.

@normanmaurer normanmaurer added a commit that closed this issue Oct 30, 2016
@vkostyukov @normanmaurer vkostyukov + normanmaurer Read if needed on ProxyHandler's handshake. Fixes #5933.
Motivation:

When auto-read is disabled and no reads are issued by a user, ProxyHandler will stall the connection on the proxy handshake phase waiting for the first response from a server (that was never read).

Modifications:

Read if needed when very first handshake message is send by ProxyHandler.

Result:

Proxy handshake now succeeds no matter if auto-read disabled or enabled. Without the fix, the new test is failing on master.
23f033a
@normanmaurer
Member

Fixed by #5939

@normanmaurer normanmaurer self-assigned this Oct 30, 2016
@normanmaurer normanmaurer added this to the 4.1.7.Final milestone Oct 30, 2016
@normanmaurer normanmaurer added the defect label Oct 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment