Recent update of urllib3 broke stream requests

I suppose you can reproduce this easily, take this url for example: http://bit.ly/1rQJ1CL

```
  File "/home/ducu/summarizer/env/lib/python2.7/site-packages/summary/__init__.py", line 280, in extract
    body = self._get_tag(response, tag_name="body", encoding=encoding)
  File "/home/ducu/summarizer/env/lib/python2.7/site-packages/summary/__init__.py", line 203, in _get_tag
    for chunk in response.iter_content(config.CHUNK_SIZE): # , decode_unicode=True
  File "/home/ducu/summarizer/env/lib/python2.7/site-packages/requests/models.py", line 664, in generate
    for chunk in self.raw.stream(chunk_size, decode_content=True):
  File "/home/ducu/summarizer/env/lib/python2.7/site-packages/requests/packages/urllib3/response.py", line 349, in stream
    for line in self.read_chunked(amt, decode_content=decode_content):
  File "/home/ducu/summarizer/env/lib/python2.7/site-packages/requests/packages/urllib3/response.py", line 501, in read_chunked
    chunk = self._handle_chunk(amt)
  File "/home/ducu/summarizer/env/lib/python2.7/site-packages/requests/packages/urllib3/response.py", line 461, in _handle_chunk
    value = self._fp._safe_read(amt)
  File "/home/ducu/.pyenv/versions/2.7.8/lib/python2.7/httplib.py", line 658, in _safe_read
    chunk = self.fp.read(min(amt, MAXAMOUNT))
AttributeError: 'NoneType' object has no attribute 'read'
```


I'm sorry, I actually _can't_ reproduce this. Can you show me slightly more about how you invoked requests?


Just a sec, not that easy to reproduce indeed..
I get it via https://github.com/svven/summary with `requests` 2.10.0. No problem with 2.9.2 but I couldn't reproduce it directly with `requests`, still investigating


So, to be clear, this is definitely a bug. My suspicion is that the bug is actually in urllib3, not requests, but we'll see if we can get a better repro before concluding that.


Sorry but can't reproduce it directly, looks like it's in urllib3 indeed.
If you feel like it you can `pip install summary-extraction`, then `pip install requests --upgrade` so you get ver 2.10.0. Then

```
>>> import summary
>>> url = 'http://bit.ly/1rQJ1CL'
>>> s = summary.Summary(url)
>>> s.extract()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/ducu/.pyenv/versions/2.7.8/lib/python2.7/site-packages/summary/__init__.py", line 251, in extract
    body = self._get_tag(response, tag_name="body")
  File "/home/ducu/.pyenv/versions/2.7.8/lib/python2.7/site-packages/summary/__init__.py", line 184, in _get_tag
    for chunk in response.iter_content(CHUNK_SIZE, decode_unicode=True):
  File "/home/ducu/.pyenv/versions/2.7.8/lib/python2.7/site-packages/requests/utils.py", line 368, in stream_decode_response_unicode
    for chunk in iterator:
  File "/home/ducu/.pyenv/versions/2.7.8/lib/python2.7/site-packages/requests/models.py", line 664, in generate
    for chunk in self.raw.stream(chunk_size, decode_content=True):
  File "/home/ducu/.pyenv/versions/2.7.8/lib/python2.7/site-packages/requests/packages/urllib3/response.py", line 349, in stream
    for line in self.read_chunked(amt, decode_content=decode_content):
  File "/home/ducu/.pyenv/versions/2.7.8/lib/python2.7/site-packages/requests/packages/urllib3/response.py", line 501, in read_chunked
    chunk = self._handle_chunk(amt)
  File "/home/ducu/.pyenv/versions/2.7.8/lib/python2.7/site-packages/requests/packages/urllib3/response.py", line 461, in _handle_chunk
    value = self._fp._safe_read(amt)
  File "/home/ducu/.pyenv/versions/2.7.8/lib/python2.7/httplib.py", line 658, in _safe_read
    chunk = self.fp.read(min(amt, MAXAMOUNT))
AttributeError: 'NoneType' object has no attribute 'read'
>>> 

```

I'm using Python 2.7.8 (default, Sep  4 2015, 14:01:33) [GCC 4.8.4] on linux2
I downgraded requests to 2.9.2 for now and it works fine, not sure when I'd get the time to look into this.
Thanks anyway


Ok, I've diagnosed this. Took a bit of digging and a few leaps, but I've got it. This is a urllib3 bug, so I'll open an appropriate report over there.


Actually, before we conclude this is a urllib3 bug, we need to have a discussion about what the API promises. So let me explain what the problem is.

urllib3 attempts to ensure that connections are thrown away if a problem occurs. This is implemented using a context manager in urllib3 (`_error_catcher`) that checks certain errors and then determines whether the block terminated cleanly. If it did not, it forcibly closes the connection before returning the connection object to the pool to be reopened.

That object treats _any_ exception as an error case that leaves the connection in an indeterminate state. That is mostly a good thing, except for one particularly awkward exception: [`GeneratorExit`](https://docs.python.org/2/library/exceptions.html#exceptions.GeneratorExit). This exception is raised when the `close()` method on a generator is called, and is thrown to allow things like `finally` blocks to execute properly.

_One_ case where this is called is when a generator is garbage collected. That happens in your code, because you call `iter_content` multiple times, once for each tag you search for. When that happens you spin up several generators (`iter_content` returns a generator, and `stream` returns a generator, and `read_chunked` also returns a generator, so there's a chain of at least three generators in this case). Because you don't save the return value from `iter_content`, that generator chain gets leaked. This causes the `read_chunked` generator to throw `GeneratorExit`, which causes the urllib3 `_error_catcher` to conclude that the connection was not left in a clean state and terminates it.

There are therefore a few questions:
1. Should `_error_catcher` consider `GeneratorExit` an error case, or special case it? I'm not sure: the question is whether the connection will get cleaned up properly in situations where the generator really is leaked. Currently my assumption is that it won't, and so `GeneratorExit` really is an error case.
2. Is it safe to open multiple versions of the generators `iter_content`, `stream`, and `read_chunked`? They don't make it clear. `iter_lines` in requests is clear that it is _not_ safe to do that, but the rest are left ambiguous. We need to make a call, where I suspect the answer boils down to whether `decode_content` is `True`: if it is, there is a state object that gets lost when that generator leaks. Given that requests essentially always sets `decode_content` to `True`, I think that means it's also not safe to repeatedly call `iter_content`.

@ducu In the short term, you can fix this by saving off the result of `iter_content` somewhere on your `Summary` object and then re-using that, rather than repeatedly re-calling that method. That logic will _definitely_ work, and we can work out whether or not the alternative _should_ work.

Can I get input from @kennethreitz, @shazow, and @sigmavirus24 on this please?


> Is it safe to open multiple versions of the generators iter_content, stream, and read_chunked? 

Do you mean multiple instances? If iterating/streaming/reading is supposed to be 1:1 with what the socket does underneath, then no it's not safe unless you preload the response and cache it.

Not sure about the generator stuff.


This is about stream requests, so it means repeatedly calling `iter_content`.
Preloading the whole response defies the purpose, that's not a stream request anymore, right? I do want to load it chunk by chunk and process it iteratively because I may not need all of it. 
Or am I getting something wrong? I definitely don't understand what's with the 3 generators and how `iter_content` is different from `stream`.. so sorry but I can't help much on this issue


@ducu `iter_content` builds on top of `stream`. When I say `stream` here I don't mean the `stream` keyword argument as exposed by requests, I mean the `HTTPResponse.stream()` method exposed by urllib3. Sorry for the confusion there!

So I totally agree you want to load it chunk by chunk and process it iteratively. What I'm asking is whether we think this _is_ safe, and then whether it _should be_ safe:

``` python
r = requests.get(some_url, stream=True)
iter_one = r.iter_content()
iter_two = r.iter_content()

while True:
    print next(iter_one)
    print next(iter_two)
```

Put another way, is it safe to repeatedly call `iter_content` and throw away the generator it returns (which is what your code does)?


Ah gottit, I'll try using a single generator then and see if that works.
Thanks for the clarification, brb

On Wed, May 4, 2016 at 10:55 AM Cory Benfield notifications@github.com
wrote:

> @ducu https://github.com/ducu iter_content builds on top of stream.
> When I say stream here I don't mean the stream keyword argument as
> exposed by requests, I mean the HTTPResponse.stream() method exposed by
> urllib3. Sorry for the confusion there!
> 
> So I totally agree you want to load it chunk by chunk and process it
> iteratively. What I'm asking is whether we think this _is_ safe, and then
> whether it _should be_ safe:
> 
> r = requests.get(some_url, stream=True)
> iter_one = r.iter_content()
> iter_two = r.iter_content()
> while True:
>     print next(iter_one)
>     print next(iter_two)
> 
> Put another way, is it safe to repeatedly call iter_content and throw
> away the generator it returns (which is what your code does)?
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly or view it on GitHub
> https://github.com/kennethreitz/requests/issues/3174#issuecomment-216770410


Yep it works with a single `iter_content` generator, I'll go with this workaround.
Thanks a lot for your input guys, it's your choice whether you want to fix this urllib3 issue, otherwise I can close this.


This issue has also affected other projects: [Velociraptor](https://bitbucket.org/yougov/vr.server/commits/59f4753c6270c587f9c546f4e4a4e9274601a96e) and [sseclient](https://bitbucket.org/btubbs/sseclient/commits/857724844866adb2068a9d2b5c8c1ba06775796a). It does seem that the user's expectation is that the `iter_content` should return the same or compatible iterator on each call. It's somewhat awkward to have to store both the response and the response iterator. If for nothing else than compatibility, I suggest requests should cache the iterator per response.


Would that apply to `iter_lines` too?


@Lukasa if we decide to cache the iterators, it would. I'm not sure we should be caching the iterators though.


I can't think of an example of a project or class that provides you with an iterator that is cached. I mean, if we just look at built-ins:

``` py
a = {'1': '2', '2': '3'}
next(a.items())
next(a.items())
```

Who would expect that to work? I don't think we should be enabling this behaviour, frankly.


@sigmavirus24 You're right. I wouldn't expect your example to work, and I can see why the surprise with `iter_content()`, because it's a function, which one expects to have side effects. But it still seems valuable to have a way to hook into the stream at whatever point it's currently at. This was a feature, however undesirable, that Requests 2.9 had which Requests 2.10 does not have... and the workaround is clumsy, requiring code in two different points in the code to be coordinated.

It really would be nice if the response object exposed some sort of interface such that one doesn't have to carry both the iterator and the response around anytime streaming is in play.