[Proposed Enhancement] Consume remainder of socket in Response.close

Earlier today I was speaking with @ducu on [IRC](https://botbot.me/freenode/python-requests/2015-01-05/?msg=28835010&page=1) about a problem they're having using `rq` and `requests`. My intuition (which is really all I have to work with about this problem) is that all of the content on the response is not being consumed and people assume that doing something like:

``` py
with contextlib.closing(requests.get(url, stream=True)) as response:
    # do some stuff with response
```

The socket will close properly. If I remember correctly though, we've seen that if the socket isn't completely consumed, this won't allow the socket to be released (at least not immediately). I'm further suspicious this is the cause because killing one of the workers for `rq` lets the job resume. I wonder what people think about changing [`Response.close`](https://github.com/kennethreitz/requests/blob/7612015424c88cfd98f6be692ff994898aee1502/requests/models.py#L833) to be something like:

``` py
def close(self):
    self.content
    self.raw.release_conn()
```

The call to self.content will consume the rest of the socket (if there's anything to consume) and then we'll release the connection (which should release the socket in this case).

Thoughts? Also I'm pretty tired so I could be wrong about some of this stuff.


That's not a bad idea. Some notes:
1. `response.content` may take a very long time to return if timeouts aren't configured. Closing shouldn't hang.
2. `Response.content` may throw exceptions.

We need to decide how we're dealing with these problems.


Thank you for helping with this!

Trying to clarify a bit what's happening..
As @sigmavirus24 mentioned, I'm using [`rq`](https://github.com/nvie/rq/) and [`summary`](https://github.com/svven/summary/) to process a bunch of URLs. I start several simple (forking) [workers](http://python-rq.org/docs/workers/) so they are doing one URL per job in totally separate processes on the same machine. It all goes fine for a while and suddenly, for no apparent reason, all workers hang at the same time at this line:

https://github.com/svven/summary/blob/master/summary/__init__.py#L193

By hanging I mean suspending the running job and leaving it in the _started_ state. There's no exception whatsoever and it doesn't get passed that line so the job is not finished properly. Then it gets to the next job and does the same thing going on in this "strike" mode until the main worker process is killed. All running workers behave this way. Newly started workers perform properly though.

It's a very peculiar issue and I haven't been able to reproduce it. It just happens after a while - few hours or even days, probably depending on the number of URLs that had been processed.

Running out of available sockets may be the cause of the problem, I think it's a good assumption but I didn't verify it yet. What puzzles me is the fact that I get no exception whatsoever, the child (forked) process doing the job simply dies at that line.


> That's not a bad idea.

I'm also not entirely certain it's a good idea for the reasons you mentioned. The major benefit is that it would then match exactly the behaviour most users seem to expect from it.


Regarding the proposed solution, isn't it enough to `self.raw.release_conn()` while closing?

I don't want to be picky but reading the whole response would trash a good part of `summary`. As stated there, "summary extraction is performed gradually by parsing the HTML <head> tag first, applying specific head extraction techniques, and goes on to the <body> only if data is not complete."

Oh by the way, while iterating the HTML response, the routine starts iterating through image files from that page and does so in the same manner having `stream=True`. So basically:

``` python
with contextlib.closing(requests.get(html_url, stream=True)) as html_response:
    # do some stuff with html_response like getting the img_url
    with contextlib.closing(requests.get(img_url, stream=True)) as img_response:
        # check the image file
```

Is this bad? Still it is working fine for a while.. but it may keep sockets from being released I guess..

Anyways if you have any hints please let me know, cheers


> Regarding the proposed solution, isn't it enough to self.raw.release_conn() while closing?

No. That releases a connection back to the connection pool in urllib3 which does not necessarily close the underlying socket.

> Is this bad?

Not exactly. If you weren't using `stream=True` in the first place, you would get a performance benefit from using a session if `html_url` and `img_url` used the same scheme and host. Since you are using `stream=True` you're still creating 2 sockets (which are the expensive part of the request/response cycle). That's probably more information than you wanted to know, but I thought you might be interested anyway.

> I don't want to be picky but reading the whole response would trash a good part of summary.

There isn't a really good way to forcibly close the socket otherwise. Not without digging 6 or 10 layers deep into the response object from urllib3 to find the socket. I've been down those layers before, and most of the paths are through internal (private) methods and attributes which no one should be relying on for any reason.


Also, I'd expect the 'running out of sockets' case not to be a 'block forever' error mode, but a 'die instantly' mode. If you hit your ulimit I'm pretty sure you get killed.


Yes that's exactly what happens - it doesn't 'block at all', but it just 'dies instantly'.

I'll try to monitor the sockets and verify this assumption, but if 'running out of sockets' is the reason, how can I prevent this from happening? Isn't there a way to get an exception in this case? To me this looks like a bug, especially if the sockets are not released in case of partial requests.get with stream=True.


I wonder if calling `select` and checking if the socket is marked readable is a good idea. If it is we can pre-emptively close it, because either there's still data on it (and so it hasn't been exhausted) or it was closed remotely (and so closing it here is a no-op). @sigmavirus24?


(Note that doing this might belong in urllib3: @shazow?)


Maybe I'm missing something, but shouldn't we close the socket on [`Response.close()`](https://github.com/kennethreitz/requests/blob/7612015424c88cfd98f6be692ff994898aee1502/requests/models.py#L833)? 
I mean even for streaming requests, closing the response means one doesn't care about the rest of the data, so why keep the socket open?


> I wonder if calling select and checking if the socket is marked readable is a good idea.

Isn't select a problem on Windows?

> Maybe I'm missing something, but shouldn't we close the socket on Response.close()? 

Quoting myself from a few comments up:

> There isn't a really good way to forcibly close the socket otherwise. Not without digging 6 or 10 layers deep into the response object from urllib3 to find the socket. I've been down those layers before, and most of the paths are through internal (private) methods and attributes which no one should be relying on for any reason.

Further, `summary` will probably already get "trashed" if at any point you receive a redirect that has a body. We use the same logic as I'm proposing for `Response.close` when following redirects to prevent a socket leak in the case where `stream=True`.


Select is fine, poll is nonexistent on Windows.


@sigmavirus24: Ok I understand there's no easy way to access the socket..
But in my opinion, reading the whole content of the response when `stream=True` defies the purpose of having the streaming feature, isn't it?

Anyway if `requests` follows the redirects there's no problem with `summary`. It actually follows http_equiv_refresh "redirects" as well: https://github.com/svven/summary/blob/master/summary/__init__.py#L209

Getting back to the actual issue..
I'm trying to reproduce the problem like this: https://gist.github.com/ducu/a684c1e96afdaf2c5657

Do you think that [`psutil.Process.connections`](https://pythonhosted.org/psutil/#psutil.Process.connections) would show leaking sockets? Didn't try it properly yet but seen it running on Windows and Ubuntu, interesting how they behave differently, but no signs of leaking sockets for ~300 URLs.
I'll try it out on OS X tonight where I experienced the "worker strike" issue, and I'll feed it more URLs.
Am I on the right track here?


> Select is fine, poll is nonexistent on Windows.

Right. I always think poll is affected too. Sorry.

> Anyway if requests follows the redirects there're no problem with summary.

If the request is empty there's no problem for you. If it isn't empty then we have to consume the entire socket which is exactly what I'm proposing we do here to fix it. So if you're encountering redirects in any number and summary isn't having problems with them, then this solution shouldn't "trash" summary either. Have you done any performance benchmarks to see if this has a realistic effect on summary's performance or is this a pre-benchmark optimization?


> Do you think that psutil.Process.connections would show leaking sockets? 

Judging by the documentation it should show sockets that are potentially still open. I've had mixed luck with psutil in trying to reproduce other things though that system tools generally seem to handle better.

> Didn't try it properly yet but seen it running on Windows and Ubuntu, interesting how they behave differently, but no signs of leaking sockets for ~300 URLs.

Didn't you say this took days to occur? I doubt it would appear overnight if you've only encountered it after days of summary running.

> I'll try it out on OS X tonight where I experienced the "worker strike" issue, and I'll feed it more URLs.
> Am I on the right track here?

It would seem so, but I'd rather keep the discussion of the enhancement at the core of this issue. If you want help debugging this, we can do it in IRC.


> Have you done any performance benchmarks to see if this has a realistic effect on summary's performance or is this a pre-benchmark optimization?

Nope, pre-benchmark optimization indeed but also empirical observation.
I know that establishing connections is the expensive part as you said, but still I'd like to avoid downloading useless content if possible.

> Didn't you say this took days to occur? 

Yes indeed, it will take a while to reproduce this, maybe I'll switch to using `summary` instead of `requests` and see the difference in socket usage. If that doesn't work I'll throw in `rq` as well.

Regarding the enhancement I can't say much and maybe I didn't quite get your proposed solution. 
But it seems that by getting the whole content you're turning a streaming request into a regular one. 


> I wonder if calling select and checking if the socket is marked readable is a good idea. If it is we can pre-emptively close it, because either there's still data on it (and so it hasn't been exhausted) or it was closed remotely (and so closing it here is a no-op)

I've always assumed that this is in effect what https://github.com/shazow/urllib3/blob/master/urllib3/util/connection.py#L12 does, among other things.

I'm not sure always consuming a socket before closing is a great idea, there could be a lot to consume or it could be a never-ending streaming connection etc.


> I'm not sure always consuming a socket before closing is a great idea, there could be a lot to consume or it could be a never-ending streaming connection etc.

Exactly my point. Any other suggestions?

—
Sent from Mailbox


> Exactly my point. Any other suggestions?

It seems this thread is on the right track. Closing a socket should be sufficient, need to figure out why it's not working as expected in this case.


Sorry guys, it's not what we assumed, the sockets are being closed properly.

It's a different problem which has to do with `requests` being used in both a main process and a forked process -- the `rq` worker script. If the network goes down even for a short while, `requests` raises few exceptions then it becomes unusable and the forked processes get killed instantly if they try to use it. This messes up `rq` jobs by leaving them in the 'started' status, so this is also an `rq` issue (https://github.com/nvie/rq/issues/473).

I've put up a [gist](https://gist.github.com/ducu/ee8c0b1028775df6c72e) to reproduce the problem, but I'll create a new bug ticket and describe it there.

Thanks a lot for your support