Connection keep alive issue

_Maybe it is not the right place to report this, its one of my first issue report_

Winows 7
Python 3.4
Request 2.3

In requests' main site it's said:

> > connection keepalive should be 100% automatic.

However in my case:
- on a ssl connection
- throught http proxy

I had to add the `"connection" : "keep-alive"` header.
But it took me so much time to find it because the exceeption raised was a `requests.exceptions.ProxyError`

So i went into the code and find that:

``` python
httplib_response = conn.getresponse()
```

in `_make_request()` function in `connection_pool.py` raise a `SocketError` wich is converted to a `ReadTimeoutError` : all correct for now since no connection then we come to a timeout.

But then this error is rewrap to something incorrect in `connectionpool.py`.

``` python
        except (TimeoutError, HTTPException, SocketError) as e:
            if conn:
                # Discard the connection for these exceptions. It will be
                # be replaced during the next _get_conn() call.
                conn.close()
                conn = None

            if not retries:
                if isinstance(e, TimeoutError):
                    # TimeoutError is exempt from MaxRetryError-wrapping.
                    # FIXME: ... Not sure why. Add a reason here.
                    raise

                # Wrap unexpected exceptions with the most appropriate
                # module-level exception and re-raise.
                if isinstance(e, SocketError) and self.proxy:
                    raise ProxyError('Cannot connect to proxy.', e)

                if retries is False:
                    raise ConnectionError('Connection failed.', e)

                raise MaxRetryError(self, url, e)

            # Keep track of the error for the retry warning.
            err = e
```

and since there is a proxy. Boom. proxy error.

So **WRONG** advice in requests site and **bug** in urllib3.
Hope it can help.


I could have sworn this was already documented (that in order to enable keep-alive you had to set that header yourself). It seems to have been lost. The documentation is only partially incorrect in this case. There is no bug in urllib3 as I understand it.


Was no critic, but an improvement suggestion.

Yes bug is stronger term but I think that rewrapping a `SocketError` to a `ProxyError` as soon as their is a proxy and even if these is no `ProxyError` can be misleading. I don't study the code deep enough but maybe adding a function that can sort of heartbeet through the ssl socket can determine if it is really a proxy error or just the connection is closed.

Maybe it can be possible in future request version to add this header as default. I think that all browser nowdays use persistent request, isn't it.
But maybe request is no the place to do it and it shall be done directly in `http.client` _(client.py)_ as it is already been done for Accept-Encoding:

``` python
if not skip_host:
    # this header is issued *only* for HTTP/1.1
    # connections. more specifically, this means it is
    # only issued when the client uses the new
    # HTTPConnection() class. backwards-compat clients
    # will be using HTTP/1.0 and those clients may be
    # issuing this header themselves. we should NOT issue
    # it twice; some web servers (such as Apache) barf
    # when they see two Host: headers

    # If we need a non-standard port,include it in the
    # header.  If the request is going through a proxy,
    # but the host of the actual URL, not the host of the
    # proxy.

    netloc = ''
    if url.startswith('http'):
        nil, netloc, nil, nil, nil = urlsplit(url)

    if netloc:
        try:
            netloc_enc = netloc.encode("ascii")
        except UnicodeEncodeError:
            netloc_enc = netloc.encode("idna")
        self.putheader('Host', netloc_enc)
    else:
        if self._tunnel_host:
            host = self._tunnel_host
            port = self._tunnel_port
        else:
            host = self.host
            port = self.port

        try:
            host_enc = host.encode("ascii")
        except UnicodeEncodeError:
            host_enc = host.encode("idna")

        # As per RFC 273, IPv6 address should be wrapped with []
        # when used as Host header

        if host.find(':') >= 0:
            host_enc = b'[' + host_enc + b']'

        if port == self.default_port:
            self.putheader('Host', host_enc)
        else:
            host_enc = host_enc.decode("ascii")
            self.putheader('Host', "%s:%s" % (host_enc, port))

# note: we are assuming that clients will not attempt to set these
#       headers since *this* library must deal with the
#       consequences. this also means that when the supporting
#       libraries are updated to recognize other forms, then this
#       code should be changed (removed or updated).

# we only want a Content-Encoding of "identity" since we don't
# support encodings such as x-gzip or x-deflate.
if not skip_accept_encoding:
    self.putheader('Accept-Encoding', 'identity')

# we can accept "chunked" Transfer-Encodings, but no others
# NOTE: no TE header implies *only* "chunked"
#self.putheader('TE', 'chunked')

# if TE is supplied in the header, then it must appear in a
# Connection header.
#self.putheader('Connection', 'TE')
```


This seems like a major bug to me — has this always been the case?

Keep alive should absolutely be automatic, just like it is in a web browser. 

Is there something preventing us from doing that?


I wonder if @kevinburke or someone else removed it in one of their PRs modifying the default headers we set


Uh, by default we emit 'Connection: close':

``` python
>>> r = requests.get('http://httpbin.org/get')
>>> print r.json()['headers'][u'Connection']
close
```

That seems like it's the source of the problem. =P


Hi,
indead all the browser nowadays include by defalt this header.
A solution may be to add the kwargs `{keepalive=True}` that automaticaly put the header.
If the user need to desctive it he can just set it back to False.

Should this be fix in request lib or direcly in `http.client`


Suspect requests should just set the header, the user can always override it.

Maybe make it a default field in `Session.headers`?


Are we sure this didn't change in urllib3 at some point?

I'm okay with making it default, but feel very confident that this used to be something the user _had_ to set in order to have the behaviour. If I remember correctly, setting the header is all the user needs to do.


I don't think so. Our docs strongly suggest it's always been automatic. It's possible urllib3 changed it, I suppose.


So looking at our git history makes me think this used to be part of 0.x's config options and that urllib3 made it possible to not have to set anything on our end. 1c20d01f10e13a0c2f9348b062d04d5f222542db seems to indicate, however, that we never set the header and users would need to set it themselves. All in all, this makes me think the documentation was strongly misleading (and a bit contradictory) in assuming it would be automatic.


Well, not setting it by default make very hard to find problems:
1. there is no clue this can be this cause the exception is raise on ssl socket level and then badly rewritten (cf first post, in wich it appears as proxy error as soon as a proxy exists)
2. all the browser (and **also curl**) set it automaticly so for users using for instance chrome developper tools or firefox ones to watch request and then redo it, they will search for a lot of time before thinking to this


I think Heroku may be emitting that header when it passes the connection to
your app

``` python
r = requests.get('http://httpbin.org/headers'); r.request.headers
{'Accept-Encoding': 'gzip, deflate', 'Accept': '*/*', 'User-Agent':
'python-requests/2.3.0 PyPy/2.3.1 Darwin/13.3.0'}
```

Versus

``` python
>>>> requests.get('http://httpbin.org/headers')
<Response [200]>
>>>> r = _
>>>> r.content
'{\n  "headers": {\n    "Accept": "*/*", \n    "Accept-Encoding": "gzip,
deflate", \n    "Connection": "close", \n    "Host": "httpbin.org", \n
 "User-Agent": "python-requests/2.3.0 PyPy/2.3.1 Darwin/13.3.0", \n
 "X-Request-Id": "0d7b324b-0979-44a3-823b-547484c8f51b"\n  }\n}'
```

## 

Kevin Burke
phone: 925.271.7005 | kev.inburke.com

On Tue, Aug 26, 2014 at 12:33 PM, Cory Benfield notifications@github.com
wrote:

> Uh, by default we emit 'Connection: close':
> 
> > > > r = requests.get('http://httpbin.org/get')>>> print r.json()['headers'][u'Connection']close
> 
> That seems like it's the source of the problem. =P
> 
> —
> Reply to this email directly or view it on GitHub
> https://github.com/kennethreitz/requests/issues/2169#issuecomment-53475860
> .


I set up a Flask app that shows no connection header is being sent

https://gist.github.com/kevinburke/bf5d6e30aa8588ba5b16

## 

Kevin Burke
phone: 925.271.7005 | kev.inburke.com

On Tue, Aug 26, 2014 at 2:20 PM, Kevin Burke kev@inburke.com wrote:

> I think Heroku may be emitting that header when it passes the connection
> to your app
> 
> ``` python
> r = requests.get('http://httpbin.org/headers'); r.request.headers
> {'Accept-Encoding': 'gzip, deflate', 'Accept': '*/*', 'User-Agent':
> 'python-requests/2.3.0 PyPy/2.3.1 Darwin/13.3.0'}
> ```
> 
> Versus
> 
> ``` python
> >>>> requests.get('http://httpbin.org/headers')
> <Response [200]>
> >>>> r = _
> >>>> r.content
> '{\n  "headers": {\n    "Accept": "*/*", \n    "Accept-Encoding": "gzip,
> deflate", \n    "Connection": "close", \n    "Host": "httpbin.org", \n
>  "User-Agent": "python-requests/2.3.0 PyPy/2.3.1 Darwin/13.3.0", \n
>  "X-Request-Id": "0d7b324b-0979-44a3-823b-547484c8f51b"\n  }\n}'
> ```
> 
> ## 
> 
> Kevin Burke
> phone: 925.271.7005 | kev.inburke.com
> 
> On Tue, Aug 26, 2014 at 12:33 PM, Cory Benfield notifications@github.com
> wrote:
> 
> > Uh, by default we emit 'Connection: close':
> > 
> > > > > r = requests.get('http://httpbin.org/get')>>> print r.json()['headers'][u'Connection']close
> > 
> > That seems like it's the source of the problem. =P
> > 
> > —
> > Reply to this email directly or view it on GitHub
> > https://github.com/kennethreitz/requests/issues/2169#issuecomment-53475860
> > .


> 1. all the browser (and also curl) 

@tychotatitscheff requests is not a browser nor is it curl. Some of our behaviour is informed by them because how they behave is the de facto way servers expect all user-agents to behave. Regardless, it's a moot point because we all seem to be in agreement that this should be set by default.

The discussion that should have happened here (not where it did happen) is how adding this now will affect users of requests. The fact that this likely disappeared in 1.0 means that no one has really missed this significantly in almost a year and a half (if not 2 years). That said, the consequences of forcing it in something like 2.3.1 or 2.4.0 are unclear right now. It's possible this will affect larger consumers of requests like RedHat, Rackspace, OpenStack, and Runscope. For now, the solution is fairly simple: use a `session` object on which you set `session.headers['Connection'] = 'keep-alive'`.


@kevinburke Interesting! I didn't have wireshark working at the time so I didn't check it. You're right though, we just aren't emitting it at all. Not quite so bad. =)

I'm with @sigmavirus24 on this: we should definitely set it by default, but we should hold off until at least 2.4.0 and possible until 3.0.


Change request #2186 compliant with previous comment and more human.


So as I understand, we just wait to `3.0.0` and then add to https://github.com/kennethreitz/requests/blob/master/requests/utils.py#L553 ?


@tychotatitscheff I believe that's closer to what we want to do, yeah. We'll see, it depends a bit on what @kennethreitz wants.


Also I'll be spending some time tonight checking if adding this by default will really harm any of our consumers. We've already done this once very recently by caching 301 redirects. We need to be more aware of how harmful things that seem like minor fixes really can be.


Seems legit.
Good luck and please let me know if you need my help for anything.
I just poke @shazow, so he can read the wrong (imao) rewrapping of `SocketError` to `ProxyError` (first post) and say me if i shall open an issue on https://github.com/shazow/urllib3
Cheers


Concerning #2186, the main advantage is that it allow situation like:
- one makes `keep_alive` parameter for the session
- the ones specify `keep_alive` false for one request

Like `verify`, it thus allow setings default and overrititng it just for the request. Simple and easy. 
Without that, one shall do PreparedRequest then manually deleted the header. I personnally find this way less logic but I understand the concern not polluated the API.


To override the keep alive when used as a default header, do this:

``` python
s = requests.Session()
s.headers['Connection'] = 'close'
```

Still very simple.

Remember that we always prioritise headers set by the user over ones we set ourselves.


I just feel stupid :|


@sigmavirus24 what were the results of your investigation? I intend to cut 2.4.0 shortly, which will add the proper header in the defaults. 


They were inconclusive. From what I can tell I don't think there will be problems, but I can't be 100% certain. The alternative to @Lukasa's suggestion of using `'close'` is also to set the value to `None`.


Fixed!