Skip to content
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

Proxy support for HTTPS connection pool #1050

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gustafsson
Copy link
Contributor

@gustafsson gustafsson commented May 15, 2023

Connection pools are really effective. This PR makes them work through a proxy as well.

When establishing an HTTPS connection through a proxy a connection is first made with the proxy and then a second connection, a tunnel, is established with the target host. This second connection is kept in a pool whereas the connection with the proxy is not kept in the pool.

Later, when a subsequent connection is made with the same target host it could reuse the tunnel. Unfortunately the tunnel isn't found when looking in the pool for available connections. The reason is that the sslupgrade function creates a new Connection with a different connectionkey. This mismatch between keys causes newconnection to not find the reusable tunnel connection, even though we readily keep the tunnel alive sitting there just waiting to be reused.

In this PR, newtunnelconnection uses a connectionkey that can be reused as expected.

Does this look like the right approach for this kind of functionality?

@gustafsson gustafsson changed the title Connection pools support keepalive through a proxyserver Connection pools over https through a proxyserver May 16, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 17, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (15c6451) 82.71% compared to head (044874a) 83.26%.
Report is 1 commits behind head on master.

Files Patch % Lines
src/Tunnel.jl 95.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1050      +/-   ##
==========================================
+ Coverage   82.71%   83.26%   +0.55%     
==========================================
  Files          32       33       +1     
  Lines        3054     3066      +12     
==========================================
+ Hits         2526     2553      +27     
+ Misses        528      513      -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gustafsson gustafsson force-pushed the pools-with-proxy branch 4 times, most recently from 95e344f to aefc458 Compare May 17, 2023 08:51
@gustafsson gustafsson changed the title Connection pools over https through a proxyserver Proxy support for HTTPS connection pool May 17, 2023
Creates a tunneled connection with a pool key that `aquire` can find on subsequent requests if the connection is kept alive
@gustafsson
Copy link
Contributor Author

(I've rebased since this PR was created a while ago)

This PR currently consists of 3 commits:

  1. test: reuse connection from pool I was unable to find any prior references to the pool in the test suite.

  2. broken test: https pool with proxy adds a broken test to illustrate the issue.

  3. Tunnel: newtunnelconnection is my proposed implementation to resolve the broken test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants