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

reconnect issues via CONNECT proxies #366

Closed
cg2v opened this issue Apr 3, 2014 · 12 comments
Closed

reconnect issues via CONNECT proxies #366

cg2v opened this issue Apr 3, 2014 · 12 comments

Comments

@cg2v
Copy link
Contributor

cg2v commented Apr 3, 2014

if I connect to an https server via a CONNECT proxy (ProxyManager), and either the origin server or the proxy disconnects, then subsequent requests connect directly to the origin server and attempt to use it as a proxy. This is because the first time httplib.HTTPConnection._tunnel() is called, it changes the .host and .port to be the origin server's.

I have a patch that fixes this for me, but I don't think I will be able to produce a test case that tests for this issue.
Two changes are required, and there is another that I think makes sense.

  • is_connection_dropped should return True if the socket is None
  • HTTPConnectionPool._get_conn should not reuse a dropped connection. (maybe only if it has proxies)
  • The optional bit: self.auto_open should be set to 0 after self._tunnel() is called on a connection object. This prevents httplib from reconnecting on send. Instead, it will throw NotConnected(). auto_open being 0 could also be used as a flag to trigger the rest of the behavior.

Assuming you have a functioning proxy (for testing, I set up a simple proxy on apache on localhost), the following script should fail:

p=ProxyManager(proxy_url="http://somewhere")
r=p.request('GET', 'https://api.box.com/', redirect=False, headers={'Connection':'close'})
r=p.request('GET', 'https://api.box.com/', redirect=False, headers={'Connection':'close'})

(my real code uses requests, and I'm not sure that this simple test will fail in all the same cases as my requesrs code)

@shazow
Copy link
Member

shazow commented Apr 3, 2014

Have you looked at some of the proxy-related tests in test_socketlevel.py?

(We need to break that up into smaller pieces so that it's more discoverable.)

Could you expand on why HTTPConnectionPool._get_conn should not reuse dropped connections? If there is a bug where recycled connections don't get re-tunneled properly, is it sensible to fix that instead?

@cg2v
Copy link
Contributor Author

cg2v commented Apr 3, 2014

In order to make connections retunnel properly, you'd have to modify or monkeypatch httplib

@shazow
Copy link
Member

shazow commented Apr 3, 2014

Sadness. :(

If you could find a way to write a test which illustrates the broken behaviour, that will definitely help move this along. Should be doable with the socketlevel suite we have.

@cg2v
Copy link
Contributor Author

cg2v commented Apr 3, 2014

test_socketlevel.py doesn't test CONNECT at all. But since there are working tests for SSL, I might be able to cobble something together.

@cg2v
Copy link
Contributor Author

cg2v commented Apr 4, 2014

I added a test to test_socketlevel.py. The test (TestProxyManager.test_connect_reconn) uses retries=0 because it seems that in a pure urllib3 environment, retries eventually do reconnect to the proxy, so at least part of my problem is requests aborting the first time it sees a socket.error when proxying.

urllib3 still does have a problem, since it does try to connect to the origin server at least once. (run the test with retries=1 and a tcpdump looking at port 443 on the loopback interface)

@shazow
Copy link
Member

shazow commented Apr 4, 2014

@cg2v That looks like a sensible starting point, thanks for making that effort! Would you like to migrate this conversation into a PR around that test case?

@jschneier
Copy link
Contributor

i believe this is the real issue behind #295, glad someone has a reproducible test case now

@cg2v
Copy link
Contributor Author

cg2v commented Apr 7, 2014

What should the pull request contain? I don't see how it makes sense to submit a PR that will result in the tests failing.

@shazow
Copy link
Member

shazow commented Apr 7, 2014

To start, just the failing tests of the desired behaviour. :) Following, ideally fixes for this behaviours from you or somebody else. We'll probably avoid merging the PR (or any forks of it) until it passes, though.

@schlamar
Copy link
Contributor

Ah interesting. I see a similar issue using devpi behind a proxy. I always thought this is an issue of the proxy but now I'm not sure anymore. It looks like urllib3 is resetting the connection but still using it for a new request:

2014-04-30 08:22:05,022 [DEBUG] devpi_server.extpypi: querying pypi changelog since 1076255
2014-04-30 08:22:05,023 [INFO ] requests.packages.urllib3.connectionpool: Starting new HTTPS connection (13): pypi.python.org
2014-04-30 08:22:05,821 [DEBUG] requests.packages.urllib3.connectionpool: "POST /pypi/ HTTP/1.1" 200 None
2014-04-30 08:22:05,822 [DEBUG] devpi_server.extpypi: processed changelog of size 2: set(['lisa-server'])
2014-04-30 08:23:06,345 [DEBUG] devpi_server.extpypi: querying pypi changelog since 1076257
2014-04-30 08:23:06,346 [INFO ] requests.packages.urllib3.connectionpool: Resetting dropped connection: pypi.python.org
2014-04-30 08:23:06,362 [WARNI] devpi_server.extpypi: changelog_since_serial: error HTTPSConnectionPool(host='pypi.python.org', port=443): Max retries
 exceeded with url: /pypi/ (Caused by <class 'httplib.BadStatusLine'>: '') with remote https://pypi.python.org/pypi/

@schlamar
Copy link
Contributor

#369 is fixing this issue 👍

@shazow
Copy link
Member

shazow commented Jun 3, 2014

I assume #369 fixed this. Let me know if this needs to be reopened. :)

shazow added a commit that referenced this issue Jun 3, 2014
@shazow shazow closed this as completed Jun 3, 2014
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

No branches or pull requests

4 participants