urllib3
urllib3 copied to clipboard
Retry classes should be able to modify the request headers
It would be useful for Retry
subclasses to be able to modify the request headers between retries.
This came up in a discussion over at https://github.com/google/oauth2client/pull/659. Essentially, we'd love to be able to write a Retry class that detects 401 errors, refreshes the credentials, and then updates the headers. This would reduce the code in our urlopen
implementation, but wouldn't completely obviate the need for a custom HTTP class (for that, we'd need some way to inject headers before requests).
I seem to recall that @haikuginger and I kicked around the idea of allowing for a request/response transformation pipeline: essentially, the ability to register handlers for requests/responses that would allow transformations. This would work really well for auth as well.
Given that the Retry logic doesn't help you as much as this idea does, can you give me an idea about how much help a custom Retry logic would give?
Yeah, I've got a prototype that I need to sit down with and expand a bit, but that was my first reaction as well; a pipelining class into which you can insert any number of mutators that'll take effect in order.
@haikuginger's idea sounds intriguing, I'd definitely be willing to experiment with how it'll work with auth.
It sounds like it'll allow us to get away with not needing to modify the headers in our retry class. 👍
@Lukasa to go into more detail on sort of what we'd like to have instead of our custom class, here's a sketch of what I'd love to be able to reduce our code to:
def inject_auth(credentials, http, method, url, headers, body, **kwargs):
if not credentials.valid:
credentials.refresh()
credentials.before_request(http, method, url, headers)
def add_credentials_to_http(http):
http.on_before_request(functools.partial(inject_auth, credentials))
# Retry on all methods when 401 is encountered, although we'd probably do this with a wrapping
# custom retry object to avoid clobbering user settings.
http.retries.method_whitelist = False
http.retries.status_forcelist = (401,)
@jonparrott, @Lukasa, want to take a look at this (very) rough draft of the HttpPipeline object?
https://github.com/haikuginger/httppipeline
@haikuginger that looks like it could work quite well for our purposes.