-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Fix retry counts in cached client #17104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the optional as zero retries is indistinguishable from None.
EliteTK
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of those should be better in the upstack PR: https://github.com/astral-sh/uv/pull/17105/changes#diff-554df8b1efa8f690930e0d00e1c7302751a6e9193fd812a9b1f7305859ceee29R1171-R1201
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will look at it tomorrow.
| return if total_retries > 0 { | ||
| Err(err.with_retries(total_retries)) | ||
| } else { | ||
| Err(err) | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.