-
Notifications
You must be signed in to change notification settings - Fork 240
Fix HTTP client proxy support #742
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Since the introduction of HTTP/2 support in the client, proxy handlers (requested via proxy-options) would be installed only after the HTTP version was negotiated. In case of a HTTPS connection, this would be attempted via ALPN which means the TLS handshake would have already taken place with the actual destination host, bypassing the configured proxy. This resulted in an error (see #731). With this patch, the proxy handlers are installed earlier again so that a configured proxy is also used during the TLS handshake phase.
Turns out this worked perfectly fine already.
FYI: There's a huge diff there due to wrapping + reindent, so I recommend to hide whitespace when looking at the diff once more! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok to me, but I'm rusty, and never did anything with proxies even when I was working on Aleph, so take with a grain of salt :D
49cb9d2
to
d498758
Compare
* Ensure that proxy connection errors have precedence over subsequent errors so that they don't get wrapped in e.g. SSL handshake exceptions. * Fix proxy connection timeout detection (the check wasn't correct anymore). * Propagate ProxyConnectionTimeoutException directly instead of wrapping it in another ConnectionTimeoutException * Add tests for proxy connection errors
Thanks for the great effort! I'm sorry that I didn't have time to contribute some tests. Maybe next time. Also congragulations to the 10²th tag and the 19²th closed issue! These are nice round numbers. |
Fixes #731