Conversation
1e18331 to
7571ffa
Compare
75c964a to
1faec8b
Compare
1faec8b to
f4a3fac
Compare
| Client(Error), | ||
| /// Track retries before a callback explicitly, as we can't attach them to the callback error | ||
| /// type. | ||
| Callback { retries: u32, err: CallbackError }, |
There was a problem hiding this comment.
Removed the optional as zero retries is indistinguishable from None.
EliteTK
left a comment
There was a problem hiding this comment.
I'm a little bit too unfamiliar with the underlying code to understand why you consider this to be a hack but I've given it a thorough looking over and I can't see anything that stands out.
The surrounding code may benefit from some future rework, but I haven't looked deeply into it here as it's not really related to this specific PR.
| Err(err) => { | ||
| // Check if the middleware already performed retries | ||
| total_retries += err.retries().unwrap_or_default(); | ||
| if is_transient_network_error(err.error()) { | ||
| // If middleware already retried, consider that in our retry budget | ||
| let retry_decision = retry_policy.should_retry(start_time, total_retries); |
There was a problem hiding this comment.
nitpick: I think the second comment doesn't make much sense now the addition (consideration) has moved.
| Err(err) => { | |
| // Check if the middleware already performed retries | |
| total_retries += err.retries().unwrap_or_default(); | |
| if is_transient_network_error(err.error()) { | |
| // If middleware already retried, consider that in our retry budget | |
| let retry_decision = retry_policy.should_retry(start_time, total_retries); | |
| Err(err) => { | |
| // Check if the middleware already performed retries and consider it in our retry budget | |
| total_retries += err.retries().unwrap_or_default(); | |
| if is_transient_network_error(err.error()) { | |
| let retry_decision = retry_policy.should_retry(start_time, total_retries); |
There was a problem hiding this comment.
Both of those should be better in the upstack PR: https://github.com/astral-sh/uv/pull/17105/changes#diff-554df8b1efa8f690930e0d00e1c7302751a6e9193fd812a9b1f7305859ceee29R1171-R1201
| return if total_retries > 0 { | ||
| Err(err.with_retries(total_retries)) | ||
| } else { | ||
| Err(err) | ||
| }; |
There was a problem hiding this comment.
| return if total_retries > 0 { | |
| Err(err.with_retries(total_retries)) | |
| } else { | |
| Err(err) | |
| }; | |
| return Err(err.with_retries(total_retries)); |
Since the retry count is non-optional, conditionally doing with_retries here is now somewhat meaningless.
Previously, we dropped the counts from the middleware layer, potentially doing to many retries and/or reporting too few. Not pretty but fixes the bug.
f4a3fac to
24e22f2
Compare
Previously, we dropped the counts from the middleware layer, potentially doing to many retries and/or reporting too few.
Not pretty but fixes the bug.