Surface HTTPError exception on 429

[rayluo]

Motivation: If AAD is under heavy traffic, it would return a response with HTTP 429 status code and a Retry-After header. Previously this information was not bubbled up to the app developers by ADAL Python.

Implementation: This PR provides such information by raising the underlying requests.HTTPError exception, which wraps the entire raw response so app developers have access to the response headers. The usage pattern would look like this.

import requests

try:
    # The following 2 lines mimic the potential http 429 situation encountered by ADAL
    resp = requests.get('https://httpbin.org/status/429')
    resp.raise_for_status()

    print("If you reach this line, there is no error")
except requests.HTTPError as e:
    print("Retry after %s", e.response.headers.get("Retry-After"))

[yugangw-msft]

Can we retry on half of the client applications, by default? Or your team has decided not to do it across all languages?

[yugangw-msft]

The sample provided means a client application needs to have an explicit dependency on the requests to be able to capture such exceptions which I am not sure is a good pattern. Also does the HttpError has to mean 429? Please double check.
The change LGTM otherwise.

[rayluo]

Documenting some off-line discussion.

Q1: Can we retry on behalf of the client applications, by default?

A: The retry interval could potentially be very long. That textbook example is one hour. I'm afraid there is no obvious way for a middle-tier sdk to handle a retry for that long. We have to surface that Retry-After as-is to the app level, and let the app do make a decision to delay the retry and/or inform user.

Or, you might simply choose to do nothing and let such Exception bubble up, and therefore aborting current session with an error trace. In my understanding, the original motivation to that feature is that some long-running backend process would do frequent retry, which slowed down AAD further. If your app is a CLI, and an end user sees the CLI emits an exception and then exits, he/she might be a little confusing, but at least our AAD server wouldn't be flooded. We expect such HTTP 429 error would be very rare, so the actual impact on User Experience would be minimal.

Q2: Does the HTTPError always mean 429?

A: As of now, we choose to only surface HTTPError when "429 Too Many Requests" was returned from authentication service. From the app dev's perspective, the meaningful part is the optional Retry-After header came with the HTTPError.

Q3: The sample provided means a client application needs to have an explicit dependency on the 3rd party package requests to be able to capture such exceptions which I am not sure is a good pattern.

A: The current exposing-underlying-exception approach is an unusual but deliberate decision, based on the following pros-and-cons thoughs:

• It is true that we would usually want to avoid surfacing underlying exceptions, so that we would be able to potentially switch to different underlying 3rd-party package in future, without affecting SDK users.

• In this PR's context, though, the error we are dealing with is the "HTTP 429 Too Many Requests" with an optional Retry-After http response header. It could happen not only in AAD, but also in any other web services. In other words, this situation is universal. And the app developer is probably using many different SDKs besides ADAL Python. If each SDK wraps such HTTP exception in their own way, app developers would be forced to learn and code in all those different ways. That would be tedious. Now we choose to surface the original requests.HTTPError from the popular requests library, in hoping that the app developers could potentially reuse their HTTPError handling logic in the highest level, across different SDKs, providing that most other SDKs also surface same exception purposely or let it bubble up unintentionally.

• Even in ADAL Python's case, we will eventually have an MSAL Python, which may or may NOT raise same AdalError exceptions. So, if we wrap HTTP 429 in AdalError, that would mean in future, apps (like Azure CLI) would likely need to re-implement the HTTP 429 exception handling logic. On the contrary, MSAL Python will probably still use requests, so the underlying requests.HTTPError would remain.

• This one is not really a strong supporting reason but, as a matter of fact, ADAL Python (unintentionally?) did not wrap all the underlying requests errors. So Azure CLI already relies on the requests.ConnectionError in some places. There is no point to hide 3rd-party library requests.