requests leaves a hanging connection for each request

This was already noted in #458, but deserves an extra ticket: When using the simple API (via requests.head/get/post etc.), the requests module leaves a connection open until it is garbage collected. This eats up resources on the server site and it really impolite.

How to reproduce:

Run this script (can somebody please tell me how one can embed a gist in markdown? The help page shows only the result of embedding...):

https://gist.github.com/2234274

Result on my system:

<pre>
torsten@sharokan:~/workspace/loco2-git$ python demo.py
Open sockets after 20 head requests:
COMMAND   PID    USER   FD   TYPE DEVICE SIZE/OFF NODE NAME
python  13928 torsten    3u  IPv4 616968      0t0  TCP sharokan.fritz.box:50072->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten    4u  IPv4 616972      0t0  TCP sharokan.fritz.box:50073->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten    5u  IPv4 616976      0t0  TCP sharokan.fritz.box:50074->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten    6u  IPv4 616980      0t0  TCP sharokan.fritz.box:50075->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten    7u  IPv4 616984      0t0  TCP sharokan.fritz.box:50076->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten    8u  IPv4 616988      0t0  TCP sharokan.fritz.box:50077->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten    9u  IPv4 616992      0t0  TCP sharokan.fritz.box:50078->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten   10u  IPv4 616996      0t0  TCP sharokan.fritz.box:50079->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten   11u  IPv4 617000      0t0  TCP sharokan.fritz.box:50080->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten   12u  IPv4 617004      0t0  TCP sharokan.fritz.box:50081->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten   13u  IPv4 617008      0t0  TCP sharokan.fritz.box:50082->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten   14u  IPv4 617012      0t0  TCP sharokan.fritz.box:50083->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten   15u  IPv4 617016      0t0  TCP sharokan.fritz.box:50084->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten   16u  IPv4 617020      0t0  TCP sharokan.fritz.box:50085->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten   17u  IPv4 617024      0t0  TCP sharokan.fritz.box:50086->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten   18u  IPv4 617028      0t0  TCP sharokan.fritz.box:50087->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten   19u  IPv4 617032      0t0  TCP sharokan.fritz.box:50088->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten   20u  IPv4 617036      0t0  TCP sharokan.fritz.box:50089->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten   21u  IPv4 617040      0t0  TCP sharokan.fritz.box:50090->bk-in-f94.1e100.net:www (ESTABLISHED)
python  13928 torsten   22u  IPv4 617044      0t0  TCP sharokan.fritz.box:50091->bk-in-f94.1e100.net:www (ESTABLISHED)
Garbage collection result: 1700
Open sockets after garbage collection:
</pre>


I think the sockets should be closed immediately when the session is unrefed. FYI, I found this when testing a local webservice by running it in CherryPy which defaults to 10 threads - after 10 requests, the web server was blocked.


Digging further, this seems due to a reference loop and is really a problem inherited from httplib: The HTTPResponse keeps a reference to the file object as _fileobject. I would argue that that reference should be dropped after the complete response was read (it should not be possible to read from the socket of a pooled connection using some old response).


Okay, and the reason this shows up is because there is a reference cycle between the request and response object.
Otherwise, when the response object is dropped, the file would be closed. Due to the ref cycle, this can only happen on the next garbage collection.


Is the the same issue as in #239?


@shazow any thoughts?


Are we talking about urllib3 request/response objects, or requests request/response objects?

urllib3 cleans up in the [response.release_conn](https://github.com/shazow/urllib3/blob/master/urllib3/response.py#L96) method. The connection gets None'd for the exact reason that @Bluehorn mentioned. We could also None the `self._fp` but that's more for custom interfaces; I don't think httplib uses it.

We can definitely be more aggressive with closing connections in the cleanup of Requests's context manager. Right now to manually close connections, you'd need to iterate for each pool and pop its queue of connections and close them if they're open. If that's what we want to do, then I can definitely add a helper in urllib3 for "closing pools" which will do that in one call.


Another approach would be to break the hard cycle between response and request by changing req.response to a weakref proxy before returning it from the session: http://dpaste.org/GOqzA/.


@bluehorn's test continues to reproduce in trunk for me. Thanks!

Here's something else interesting. If I change the test like so:

``` diff
diff --git a/conntest.py b/conntest.py
index 3eac6c7..ec30d3f 100644
--- a/conntest.py
+++ b/conntest.py
@@ -2,8 +2,10 @@ import gc, os, subprocess, requests

 gc.disable()         # Just to make sure - I have seen this with gc enabled

+s = requests.session()
+
 for x in range(20):
-    requests.head("http://www.google.com/")
+    s.head("http://www.google.com/")
```

I get the same result, 20 leaked `ESTABLISHED` connections. Are we sure that keep-alive is working?


Sorry, documentation-reading fail. I wasn't consuming the content of the response objects. If I do that, things work as expected, connections get reused, and in the end only one is left alive.


Here's my understanding of this issue so far:
1. @Bluehorn and @gsakkis are right: the root problem is the reference cycle between `requests.Request` and `requests.Response`.
2. Breaking this cycle in a crude way, e.g., removing the `request` member of the `Response` class, fixes @Bluehorn's test case.
3. Although it might be possible to improve the way Requests uses urllib3, that can't fix the test case, because by default Requests returns the `Response` object with the socket still open and the body unread. Therefore, we rely on garbage collection to close the socket when clients don't read all the data. (Technically, since the test case is doing a `HEAD` request, the implementation of `requests.head` could be changed to close the socket. But the problem would still exist for the analogous test case calling `requests.get`.)

So here are the three viable approaches that occur to me:
1. Replace `Response.request` with a `weakref.proxy`, as @gsakkis suggested. This breaks the following test: `r = requests.get(httpbin('get)); assert r.request.sent` (because `r.request` has been garbage-collected already, so we get "ReferenceError: weakly-referenced object no longer exists"). Anyway, this involves a smallish API breakage.
2. Remove `Response.request`. This has the advantage of not using weak references. This breaks the above test and also breaks `HTTPDigestAuth`, which uses the `response` hook and expects the `Response` object it operates on to have a `request` member. We could work around this by changing the hook API, but either way, it seems like a larger API break.
3. Enable `prefetch` and disable `keep_alive` by default, and then ensure that all the data is read and the socket is closed before we return to the client.

Random thought: it seems like keep-alive should be off by default. Outside the context of a session, we have no way to reuse the connection anyway.


Heh, we could remove `Response.request` in general, but add it back in for the duration of the hook invocation. This makes #2 look more like #1, in terms of API breakage.

https://gist.github.com/2621434


In favor of option 3: any solution that relies on eager garbage collection won't work with PyPy.


Once upon a time `prefetch` was on by default, we went through significant lengths to fix that. :)

Also it's pretty rare that you'll only make 1 request to a host in a script, so having `keep_alive` on by default is a nice feature. Otherwise we'd be content using urllib{1,2} with a thin API wrapper.


@shazow 

Can you clarify the reasons for defaulting `prefetch` to off instead of on?

So --- defaulting `keep_alive=True` in the context of a session makes sense. But in the context of the vanilla API, that is to say, a plain `requests.get` or `requests.post`, there doesn't seem to be any mechanism for reusing the keep-alive connections, because an ephemeral `Session` object is created and it is given an ephemeral `PoolManager` in `init_poolmanager()`.


Perhaps the original reason was that it made urllib3 and requests perform better in naive benchmarks. Upon further consideration, it also made sense to fetch only the data we're actually using, especially for large responses when we only really care about headers. Someday in the future when we add ETAG-based caching, this will be a necessity. Overall, lots of upsides, no real downsides (until now).

I'm not super familiar with the Requests code in this context, but I'd imagine we'd want a reused static PoolManager instance for these things.


A static or module-level mechanism for sharing and reusing the connections would make sense. On the other hand, it would significantly worsen the current "gotcha" with leaked connections, because if a client doesn't call `.content`, those connections will remain reserved in the static pool forever --- not even GC will be able to reclaim them.


What if we made an explicit `del` call on the connection when [the LRU](https://github.com/shazow/urllib3/blob/master/urllib3/_collections.py) invalidates the object?


Then we'd just leak connections up to the size of the LRU cache (10 by default?).

My vote would be to enable `prefetch=True`, keep `keep_alive=True`, and add a module-level connection pool for reusing connections. I don't think this would break a future implementation of ETAG caching; we could just check the ETAG before prefetching and hang up the phone if the data is cached.


Is an O(1) leak really a leak? ;)

I'm -0 on `prefetch=True` just because I feel the current settings cover the +80% use case, but I wouldn't shed any tears. Up to @kennethreitz, I'd say.


In the real world, everything is a constant factor ;-)


Module-level connection pool doesn't sound like a good idea to me. Setting `prefetch=True` just because that's easier :) doesn't sound good, either. Just saying.


@piotr-dobrogost if we don't prefetch, I don't see a good solution that works on all platforms, including those like PyPy that lack eager garbage collection.

For comparison, here's something that always infuriated me about urllib2:

``` python
try:
    fileobj = urllib2.urlopen(MY_URL)
    text = fileobj.read()
finally:
    fileobj.close()
```

Seems like the 80%, batteries-included thing is to have an API that reads the whole page in a fire-and-forget way.


A module-level connection pool? Absolutely not. That's what `session` is for.


That makes sense, as long as we turn off `keep_alive` by default. I think:
1. Default `prefetch=True`
2. Default `keep_alive=False` (unless we're in an actual `Session`)
3. Ensure the connections are being closed

This will resolve the connection leak. Optionally, we could also break the reference cycle, as a memory-usage optimization for CPython.


All requests are made within a session.


Right. For "actual Session", read "non-ephemeral Session", i.e., "Session created by the client with the expectation of reuse."


But I guess this doesn't answer all the questions about expected behavior of `Session` objects. I think something like this:
1. A non-ephemeral `Session` should send `Connection: keep-alive` by default, and retain connections in a pool
2. On CPython, the `Session` object should not be involved in any reference cycles, and those connections should be disposed of when the object goes out of scope
3. For PyPy etc., there should be a `Session.dispose()` and/or a context manager that manually closes the connections


It shouldn't matter if it's ephemeral or not. When the session dies (and the connections therein), so should the connection pool. 

There should be no specific behavior for different platforms.


Well, we wouldn't special-case behavior inside Requests itself based on platforms. CPython would do the right thing whether or not the client called `dispose()` (which is what CPython users probably expect --- instantaneous scope-based GC). PyPy users would have to ensure that they called `dispose()`.

As for why it would matter whether the session is ephemeral or not, @craigholm makes a good point on #458: a client that does not support persistent connections "must" send `Connection: close`. If we're creating the session just to destroy it, that's essentially not supporting persistent connections. (Technically, you can get the session as `resp.request.session` and reuse it, but if we get rid of the reference cycle, that would also go away.)


`Session` objects actually already have a context manager API, it just doesn't do anything. Let's put it to work :)


@slingamn keep-alive should be on, we're using it for 301s and the like.