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

Retry bad nonce errors with a new nonce #65

Merged
merged 1 commit into from
Sep 17, 2024
Merged

Retry bad nonce errors with a new nonce #65

merged 1 commit into from
Sep 17, 2024

Conversation

nick96
Copy link

@nick96 nick96 commented Sep 16, 2024

I'm putting this up as a possible solution for handling bad nonce errors. The new BytesBody implementation feels a bit weird but I couldn't think of a better way that wouldn't have wide reaching interface changes. I think it makes sense to do the retry within Client so that everything benefits from it. The main thing this is missing is some backoff, though that may not even be necessary because we're not retrying due to a network or server side error.

Fixes: #62

@nick96
Copy link
Author

nick96 commented Sep 16, 2024

Just realised that I don't need to do the clone of BytesResponse.parts at all. I'll squash the commits into one much more descriptive one if you approve this!

Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great, thanks! A whole bunch of minor suggestions follow, please squash everything into one commit which I think I could release shortly.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@djc djc marked this pull request as ready for review September 16, 2024 14:16
@djc
Copy link
Owner

djc commented Sep 16, 2024

This is great, thanks!

@nick96
Copy link
Author

nick96 commented Sep 16, 2024

Sorry, I didn't realise you were actively reviewing and force pushed a change to use the Replay-Nonce header.

@djc
Copy link
Owner

djc commented Sep 16, 2024

No problem!

src/lib.rs Outdated
Comment on lines 517 to 521
nonce = response
.parts
.headers
.get("Replay-Nonce")
.and_then(|v| v.to_str().map(|s| s.to_string()).ok());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is nonce_from_response()? Can we use that here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it is. Done!

Retry `urn:ietf:params:acme:error:badNonce` errors as they are defined
as retryable[^1]. Requests failing with a bad nonce error will be
retried three time retried three times before the failure is returned
to the caller.

This implenments the `BytesBody` trait for `bytes::Bytes` as we need
to consume the response body to be able check if the failure was due
to a bad nonce which required parsing the body. The body is only
consumed if the response status is bad request, in all other cases the
body is still lazily consumable.

[^1]: https://datatracker.ietf.org/doc/html/rfc8555/#section-6.5
@djc djc merged commit e83836b into djc:main Sep 17, 2024
8 checks passed
@djc
Copy link
Owner

djc commented Sep 17, 2024

Thanks!

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.

Graceful handling of badNonce error, retry with new nonce
2 participants