Feat: add request.extensions["retry"]#75
Conversation
…uest, also set this object on request for introspection purposes
…xtensions["retry"] - allows more easily override Retry configuration parameters per request to pass it like Client.get(extensions={"retry": transport.retry.copy_with(...)}) (signature like httpx.URL.copy_with, typing approach with _UNSET from httpx._config)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #75 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 1001 1065 +64
=========================================
+ Hits 1001 1065 +64
🚀 New features to boost your workflow:
|
will-ockmore
left a comment
There was a problem hiding this comment.
Thanks again @vgavro for this, appreciate the though that you've taken here. Splitting my feedback by the two halves:
copy_with(): This is great. It's the right fix for the field-list duplication in increment(). Two things I'd like:
- add a test that round-trips every
__init__field throughcopy_with()so the two signatures can't silently drift - we'll need to thread validate_response through once #74/#75 ordering is sorted.
request.extensions["retry"]: I'd like to take this as a separate PR so we can get the semantics right. The blocker for me is that the transport writes the incremented retry back into request.extensions. Request objects can be reused, so a reused request comes back with a depleted budget. Can we
- read the override in from
request.extensions["retry"]but not mutate it, and - if we want post-hoc introspection of attempts_made, put the final retry on
response.extensionsinstead?
That keeps input config and output state on the right-lifetime objects.
Additionally, as it's squatting httpx's namespace here in the extensions dict, we should proceed with caution. It's a valid use case, but we may want to guard against a future version where this key is set externally.
| total=self.total if isinstance(total, _UnsetType) else total, | ||
| allowed_methods=self.allowed_methods if isinstance(allowed_methods, _UnsetType) else allowed_methods, | ||
| status_forcelist=self.status_forcelist if isinstance(status_forcelist, _UnsetType) else status_forcelist, | ||
| retry_on_exceptions=self.retryable_exceptions | ||
| if isinstance(retry_on_exceptions, _UnsetType) | ||
| else retry_on_exceptions, | ||
| backoff_factor=self.backoff_factor if isinstance(backoff_factor, _UnsetType) else backoff_factor, | ||
| respect_retry_after_header=self.respect_retry_after_header | ||
| if isinstance(respect_retry_after_header, _UnsetType) | ||
| else respect_retry_after_header, | ||
| max_backoff_wait=self.max_backoff_wait if isinstance(max_backoff_wait, _UnsetType) else max_backoff_wait, | ||
| backoff_jitter=self.backoff_jitter if isinstance(backoff_jitter, _UnsetType) else backoff_jitter, | ||
| attempts_made=self.attempts_made if isinstance(attempts_made, _UnsetType) else attempts_made, | ||
| total_timeout=self.total_timeout if isinstance(total_timeout, _UnsetType) else total_timeout, | ||
| elapsed_sleep=self.elapsed_sleep if isinstance(elapsed_sleep, _UnsetType) else elapsed_sleep, |
There was a problem hiding this comment.
As we're using the sentinel pattern here, the more performant check is x is _UNSET instead of isinstance(x, _UnsetType)
This feature allows to:
Retryconfiguration per request, the same way asTimeoutconfiguration may be overriden per request, see https://www.python-httpx.org/advanced/extensions/Allows introspection on each request of actual retries made and time spent , because
response.request.extensions["retry"]is always set (the same way asresponse.request.extensions["timeout"]is always set, so it's consistent across httpx api expectations)This is also pretty useful with feat: Add validate_response option #74 - considering you decide what request responses you want to pre-validate (for example, to read body in RetryTransport to retry timeouts), and what requests you will use with streaming.
NOTE - these two merge requests are in conflict, some of them should be merged first, and then i'll adopt other IF you're willing to merge both hopefully.
NOTES ON SOME DESIGN DECISIONS:
Retry.copy_withwas implemented for easier use to copy global retry configuration with per-request retry configuration - naming likehttpx.URL.copy_with, but instead of**kwargsin signature we're using full typing with_UNSETsentinel -for better type checker autocompletions - same pattern used inhttpx._config