Skip to content
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

Add throttling support using retry-after headers #1517

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

johnbatty
Copy link
Contributor

@johnbatty johnbatty commented Dec 17, 2023

Fixes: #1516

Updated azure_core retry policy to implement throttling by looking for and using the values returned by a server via retry-after, retry-after-ms or x-ms-retry-after-ms headers when the server returns 429 (TooManyRequests) or 503 (ServerUnavailable).

Details:

  • There was already some parsing of the retry-after headers in lro.rs (long-running operations).
    • I moved this into the retry_policies module, as I felt this was probably a more appropriate home for it.
    • The existing code had a bug where it was treating the "ms" variant values as seconds rather than milliseconds. I've fixed this.
    • The existing code also looked for the headers in a different order to the Javascript SDK. I've switched this around to match - now looking for the <xxx>-ms headers before the retry-after header.
  • I modified the RetryPolicy trait wait(...) method to have an additional retry_after parameter, to enable passing in of retry-after duration extracted from the headers (if any):
    async fn wait(&self, _error: &Error, retry_count: u32, retry_after: Option<Duration>)
  • I updated the common retry policy code to extract any retry-after header value when the server response is 429 or 503. Then when calculating the sleep time for any retry policy in the wait(...) method it takes the maximum of the retry-after value (if any) and the calculated policy value.
  • I added some unit tests for the new code.

@johnbatty
Copy link
Contributor Author

johnbatty commented Dec 17, 2023

@reillysiemens This may be of interest, given your comments in #1510

This obviously doesn't do anything to support the additional Azure DevOps throttling headers that you wanted to add (which are more about monitoring/controlling request rate to avoid hitting server throttling), but does at least provide appropriate throttling delays and retries when the server returns TooManyRequests. Once this change is in azure_core, it will get picked up by the next release of azure-devops-rust-api.

@demoray demoray merged commit d561a6a into Azure:main Dec 18, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server retry-after throttling headers are ignored
2 participants