Retries logic

Currently our documentation tells user that by creating a custom `HTTPAdapter` they can pass `max_retries=n` and that will apply to connection and read retries. This is a lie at the moment (and has been probably since 2.4.0 was released with the new version of urllib3 that added the `Retry` class).

In order to preserve backwards compatibility (and have passing tests) I remember upgrading the logic [here](https://github.com/kennethreitz/requests/blob/master/requests/adapters.py#L361) to pass `read=False` because some where we expected a plain `ReadTimeout` exception we were now getting a `MaxRetryError`. This, however, means that we have broken what we promised in the documentation (and what was likely present in even 2.3.0).

At the moment we do not support passing a `Retry` instance from `urllib3`. I'd like to propose the following changes:
- In `requests.adapters`, `DEFAULT_RETRIES` becomes a sentinel object, e.g., `DEFAULT_RETRIES = object()`.
- In `HTTPAdatper.__init__`, we check for the sentinel object and if so set `self.max_retries` to `Retry(0, read=False)`. Otherwise, we do `self.max_retries = Retry.from_int(max_retries)`

This should fix preserve the default existing behaviour that specifying `read=False` preserved but also allow users to do one of two things:
1. Import `Retry` from urllib3, construct their own `Retry` instance and then pass it as `max_retries`
2. Specify a number for both connection and read retries.

Does this sound like a fair proposal?

**Edit** I forgot to mention that @txtsd brought this to our attention in IRC. They get the credit for reporting this. Thank you @txtsd 


If you do `self.max_retries = Retry.from_int(max_retries)` doesn't this mean that you can only pass an integer number of retries?


I realize that using `0` will make it a magic number in this case. Perhaps on the sentinel object we set a `value`, e.g.,

``` python
DEFAULT_RETRIES = object()
DEFAULT_RETRIES.value = 0

# snip

class HTTPAdapter(BaseAdapter):
    def __init__(...):
        if max_retries is DEFAULT_RETRIES:
            self.max_retries = Retry(DEFAULT_RETRIES.value, read=False)
        else:
            self.max_retries = Retry.from_int(max_retries)
```

This smells a bit to me because then someone might expect this to have the same behaviour as the else clause:

``` python
import requests

requests.adapters.DEFAULT_RETRIES.value = n
```


> If you do self.max_retries = Retry.from_int(max_retries) doesn't this mean that you can only pass an integer number of retries?

If you look at [`Retry.from_int`](https://github.com/shazow/urllib3/blob/master/urllib3/util/retry.py#L145) it accepts an instance of `Retry` as well as `None`.


ah.


Sounds fair.
EDIT: And thank you :)


@kevinburke always read the source ;)


Using this patch I see this behaviour:

``` pycon
requests (retries-take-2) python
Python 2.7.7 (default, Jun 14 2014, 23:12:13)
[GCC 4.2.1 Compatible Apple LLVM 5.1 (clang-503.0.40)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import requests
>>> r = requests.get('https://httpbin.org/delay/20', timeout=5)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "requests/api.py", line 59, in get
    return request('get', url, **kwargs)
  File "requests/api.py", line 48, in request
    return session.request(method=method, url=url, **kwargs)
  File "requests/sessions.py", line 451, in request
    resp = self.send(prep, **send_kwargs)
  File "requests/sessions.py", line 557, in send
    r = adapter.send(request, **kwargs)
  File "requests/adapters.py", line 425, in send
    raise ReadTimeout(e, request=request)
requests.exceptions.ReadTimeout: HTTPSConnectionPool(host='httpbin.org', port=443): Read timed out. (read timeout=5)
>>> s = requests.session()
>>> a = requests.adapters.HTTPAdapter(max_retries=3)
>>> s.mount('https://', a)
>>> r = s.get('https://httpbin.org/delay/20', timeout=2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "requests/sessions.py", line 463, in get
    return self.request('GET', url, **kwargs)
  File "requests/sessions.py", line 451, in request
    resp = self.send(prep, **send_kwargs)
  File "requests/sessions.py", line 557, in send
    r = adapter.send(request, **kwargs)
  File "requests/adapters.py", line 416, in send
    raise ConnectionError(e, request=request)
requests.exceptions.ConnectionError: HTTPSConnectionPool(host='httpbin.org', port=443): Max retries exceeded with url: /delay/20 (Caused by ReadTimeoutError("HTTPSConnectionPool(host='httpbin.org', port=443): Read timed out. (read timeout=2)",))
```

I'll push this to a branch on sigmavirus24/requests called retries-take-2 if anyone wants to toy with it.


Side note, this also works with custom `Retry` objects:

``` pycon
>>> from requests.packages.urllib3.util.retry import Retry
>>> r = Retry(5, read=1)
>>> import requests
>>> s = requests.session()
>>> a = requests.adapters.HTTPAdapter(max_retries=r)
>>> s.mount('https://', a)
>>> s.get('https://httpbin.org/delay/20', timeout=2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "requests/sessions.py", line 463, in get
    return self.request('GET', url, **kwargs)
  File "requests/sessions.py", line 451, in request
    resp = self.send(prep, **send_kwargs)
  File "requests/sessions.py", line 557, in send
    r = adapter.send(request, **kwargs)
  File "requests/adapters.py", line 416, in send
    raise ConnectionError(e, request=request)
requests.exceptions.ConnectionError: HTTPSConnectionPool(host='httpbin.org', port=443): Max retries exceeded with url: /delay/20 (Caused by ReadTimeoutError("HTTPSConnectionPool(host='httpbin.org', port=443): Read timed out. (read timeout=2)",))
```


I'm happy with this approach. :cake:


Converted this to a PR with the changes on my branch.


prolly need to update the docs about max_retries to document the new type parameter


@kevinburke how does "If you need to provide granular detail about what to retry and how many times to retry it, import urllib3's `Retry` class and pass that instead." sound?


Good! Id maybe pass a link to the object or the urllib3 docs too

On Wednesday, September 10, 2014, Ian Cordasco notifications@github.com
wrote:

> @kevinburke https://github.com/kevinburke how does "If you need to
> provide granular detail about what to retry and how many times to retry it,
> import urllib3's Retry class and pass that instead." sound?
> 
> —
> Reply to this email directly or view it on GitHub
> https://github.com/kennethreitz/requests/pull/2216#issuecomment-55174200
> .

## 

## 

Kevin Burke
phone: 925.271.7005 | twentymilliseconds.com


Yeah I was thinking of linkifying 'Retry'.


This isn't quite right because now any retry error gets reraised as a ConnectionError. I'm working on a fix.


@kevinburke I would guess that `MaxRetryError` inherits from `ProtocolError` somewhere. While I have no strict opinions about the exception hierarchy in urllib3, this could simply mean that we need to re-order our except clauses in requests.


There's also a small amount of weirdness in the way urllib3 raises MaxRetryErrors that should get resolved before determining the logic in Requests - see https://github.com/shazow/urllib3/pull/464 for more info.


I think the sentinel object is unnecessary. Otherwise, +1


@kennethreitz I removed the sentinel object and added a quick test. @kevinburke also pointed out that it makes sense for this to be able to raise a new exception since people will not run into this if they're not using the new logic.


LGTM!

(_pretends to know how to add a cake emoji_)

On Friday, October 24, 2014, Ian Cordasco notifications@github.com wrote:

> @kennethreitz https://github.com/kennethreitz I removed the sentinel
> object and added a quick test. @kevinburke https://github.com/kevinburke
> also pointed out that it makes sense for this to be able to raise a new
> exception since people will not run into this if they're not using the new
> logic.
> 
> —
> Reply to this email directly or view it on GitHub
> https://github.com/kennethreitz/requests/pull/2216#issuecomment-60470082
> .

## 

## 

Kevin Burke
phone: 925.271.7005 | twentymilliseconds.com


+1 I would love this in pip so hard.


Ping @kennethreitz ;)


@kennethreitz I've rebased this.


:shipit: 


yolo, as the kids say


why not add a parameter to `requests.api.request` method? like `timeout` parameter.


@dawncold the API is in feature freeze. Please read up on the current status of the project. We're not expanding or adding new parameters nor will we.


@sigmavirus24 Sorry, I have searched some places on documents, but found nothing, can you provide me an URL?


@dawncold searching for "Feature Freeze" in the documentation quickly reveals [this section](http://docs.python-requests.org/en/latest/dev/todo/?highlight=freeze#feature-freeze) and searching open and closed issues for (feature freeze) bring up #1165 and #1195.


@sigmavirus24 no need to be smug :)