Implement retryable calls #98

ValuedMammal · 2024-09-07T05:23:44Z

Based on #93 the PR implements retryable calls for request failure due to too many requests (429) or service unavailable (503).

Inspired by #71
h/t @e1a0a0ea

Notes

I've added the field max_retries to the Builder. max_retries is also added to each of AsyncClient, BlockingClient. I added the dependency async-std in order to have async sleep.

Instead of implementing a trait on the Request type as in #71, the approach is to add a method on the client that sends a get request to a url and returns the response after allowing for retries. I tested on the bdk wallet_esplora_* example crates against https://blockstream.info/testnet/api and it seemed to resolve the 429 issue.

coveralls · 2024-09-07T05:30:18Z

Pull Request Test Coverage Report for Build 10962612990

Details

137 of 175 (78.29%) changed or added relevant lines in 3 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.6%) to 88.071%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/lib.rs	1	5	20.0%
src/blocking.rs	19	30	63.33%
src/async.rs	117	140	83.57%

Files with Coverage Reduction	New Missed Lines	%
src/async.rs	2	80.54%

Totals
Change from base Build 10748775132:	0.6%
Covered Lines:	1041
Relevant Lines:	1182

💛 - Coveralls

- apply some standard on `Cargo.toml` deps. - minor docstring improvements, and fix missing docstrings.

- remove duplicated HTTP client code for handling GET and POST requests. - adds a few new methods to `AsyncClient` implementation, the new methods are responsible to handle common HTTP requests and parsing. It was previously duplicated throughout the Esplora API implementation, but now it follows the same approach already implemented for blocking client (`BlockingClient`).

src/blocking.rs

oleonardolima

Concept ACK

oleonardolima

It's looking good! Thanks for working on this, I just left a few comments and nits.

I wonder if we should try to add a test for the retry behavior, but unsure if it's worth it.

oleonardolima · 2024-09-20T01:16:34Z

src/async.rs

+                    if attempts < self.max_retries
+                        && RETRYABLE_ERROR_CODES.contains(&resp.status().as_u16()) =>


nit: You can apply the same approach here and extract it to is_retryable_status too.

oleonardolima · 2024-09-20T01:17:48Z

src/async.rs

+        let mut attempts = 0;
+
+        loop {
+            match self.client.get(url).send().await {


nit: You can also just propagate the error here, right? (edit: unsure if an if would be a better style)

P.S.: The same applies to the blocking function.

oleonardolima · 2024-09-20T01:22:35Z

Cargo.toml

@@ -17,6 +17,7 @@ name = "esplora_client"
 path = "src/lib.rs"

 [dependencies]
+async-std = "1.13.0"


It should be an optional dependency, and only available when using the async feature.

Also, I don't think we need all the features in its default features, and only the std feature, is that right?

I tried with only std feature and it seems that the task module is only available with "default"

Yes, looks like it's behind the default feature here using their cfg_default! macro.

ValuedMammal · 2024-09-20T15:09:39Z

Made async-std an optional dep
Small improvements to get_with_retry

@oleonardolima I haven't come up with a test for the retry behavior, although we can at least be confident that the methods that call get_with_retry still return a response

I'm testing against https://blockstream.info/api and now seeing some "500 - Internal Server Error", so I'm considering adding this to RETRYABLE_ERROR_CODES.

oleonardolima

ACK 693af39

nit: I'd just update the last commit message to something like: refactor(async,blocking): `get_with_retry` fn to follow the standard.

src/async.rs

This change also adds a new field `max_retries` to each of `Builder`, `AsyncClient`, and `BlockingClient` that defines the maximum number of times we may retry a request.

oleonardolima

ACK b96270e

notmandatory

tACK b96270e

I tested with example_esplora on mainnet (with gap-limit 100) and testnet. In both cases tests failed with the master branch version of this code and works with the fixes in this PR.

ValuedMammal · 2024-09-25T03:35:44Z

src/blocking.rs

+
+    /// Sends a GET request to the given `url`, retrying failed attempts
+    /// for retryable error codes until max retries hit.
+    pub fn get_with_retry(&self, url: &str) -> Result<Response, Error> {


Oops, I don't think it was intended for this to be pub.

bfaa9ae fix(blocking): `get_with_retry` is private method (valued mammal) Pull request description: Clean up after #98 which accidentally exposed `get_with_retry` publicly for the blocking client. ACKs for top commit: oleonardolima: ACK bfaa9ae Tree-SHA512: 33abd34e919e27069107947c34915b9de3547ab1d78291841f2052552abd23fd59621ee6ae3ed9bd8e3eda608c2e5b9285529df1dcba8a4a1705ca1f1491c136

oleonardolima added 4 commits September 9, 2024 14:56

chore(docs): minor improvements on docstrings

31dfa4b

- apply some standard on `Cargo.toml` deps. - minor docstring improvements, and fix missing docstrings.

chore(style): update new methods style

442789c

chore(style+docs): style and docs retouch

1103936

oleonardolima reviewed Sep 9, 2024

View reviewed changes

src/blocking.rs Outdated Show resolved Hide resolved

oleonardolima reviewed Sep 9, 2024

View reviewed changes

oleonardolima mentioned this pull request Sep 12, 2024

refactor(async): refactoring async client and minor documentation improvements #93

Merged

3 tasks

ValuedMammal force-pushed the feat/retryable branch from 1f67ece to e50ce73 Compare September 18, 2024 02:30

ValuedMammal marked this pull request as ready for review September 18, 2024 02:34

oleonardolima reviewed Sep 20, 2024

View reviewed changes

oleonardolima assigned oleonardolima and ValuedMammal and unassigned oleonardolima Sep 20, 2024

oleonardolima added the enhancement New feature or request label Sep 20, 2024

oleonardolima approved these changes Sep 20, 2024

View reviewed changes

ValuedMammal commented Sep 20, 2024

View reviewed changes

src/async.rs Outdated Show resolved Hide resolved

feat(async,blocking)!: implement retryable calls

b96270e

This change also adds a new field `max_retries` to each of `Builder`, `AsyncClient`, and `BlockingClient` that defines the maximum number of times we may retry a request.

ValuedMammal force-pushed the feat/retryable branch from 693af39 to b96270e Compare September 20, 2024 16:07

oleonardolima approved these changes Sep 20, 2024

View reviewed changes

notmandatory mentioned this pull request Sep 24, 2024

wallet_esplora_async returns error 429 while scanning bitcoindevkit/bdk#1120

Closed

notmandatory approved these changes Sep 25, 2024

View reviewed changes

notmandatory merged commit 1542262 into bitcoindevkit:master Sep 25, 2024
25 checks passed

ValuedMammal commented Sep 25, 2024

View reviewed changes

ValuedMammal deleted the feat/retryable branch September 25, 2024 13:16

ValuedMammal mentioned this pull request Sep 25, 2024

fix(blocking): get_with_retry is private method #100

Merged

oleonardolima mentioned this pull request Oct 2, 2024

LDK Gateway spamming esplora and killing ldk-node fedimint/fedimint#5965

Closed

tnull mentioned this pull request Oct 3, 2024

rust-esplora-client shouldn't introduce an async runtime #102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement retryable calls #98

Implement retryable calls #98

ValuedMammal commented Sep 7, 2024 •

edited

Loading

coveralls commented Sep 7, 2024 •

edited

Loading

oleonardolima left a comment

oleonardolima left a comment

oleonardolima Sep 20, 2024

oleonardolima Sep 20, 2024 •

edited

Loading

oleonardolima Sep 20, 2024

ValuedMammal Sep 20, 2024

oleonardolima Sep 20, 2024

ValuedMammal commented Sep 20, 2024 •

edited

Loading

oleonardolima left a comment

oleonardolima left a comment

notmandatory left a comment •

edited

Loading

ValuedMammal Sep 25, 2024

		if attempts < self.max_retries
		&& RETRYABLE_ERROR_CODES.contains(&resp.status().as_u16()) =>

Implement retryable calls #98

Implement retryable calls #98

Conversation

ValuedMammal commented Sep 7, 2024 • edited Loading

Notes

coveralls commented Sep 7, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10962612990

Details

💛 - Coveralls

oleonardolima left a comment

Choose a reason for hiding this comment

oleonardolima left a comment

Choose a reason for hiding this comment

oleonardolima Sep 20, 2024

Choose a reason for hiding this comment

oleonardolima Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

oleonardolima Sep 20, 2024

Choose a reason for hiding this comment

ValuedMammal Sep 20, 2024

Choose a reason for hiding this comment

oleonardolima Sep 20, 2024

Choose a reason for hiding this comment

ValuedMammal commented Sep 20, 2024 • edited Loading

oleonardolima left a comment

Choose a reason for hiding this comment

oleonardolima left a comment

Choose a reason for hiding this comment

notmandatory left a comment • edited Loading

Choose a reason for hiding this comment

ValuedMammal Sep 25, 2024

Choose a reason for hiding this comment

ValuedMammal commented Sep 7, 2024 •

edited

Loading

coveralls commented Sep 7, 2024 •

edited

Loading

oleonardolima Sep 20, 2024 •

edited

Loading

ValuedMammal commented Sep 20, 2024 •

edited

Loading

notmandatory left a comment •

edited

Loading