Repository navigation
feat: add esplora client timeout option - #1119
Arowolokehinde wants to merge 1 commit into
Conversation
|
i would like to hear what you think about this pr @reez whenever you are chanced to |
There was a problem hiding this comment.
I think we should stick to what upstream exposes. I agree it's not optimal that our Electrum and Esplora clients use different parameters but I think it's easier to stick with what they have and PR changes we want upstream.
In this case, the newest Esplora client (0.13.0) switches to bitreq and this argument becomes a Duration, same as with the Electrum client. We can fix this up and use the u8 at that point since that's how we do it on Electrum.
Otherwise once that little fix is done and the PR is rebased I think this looks ready to go.
| /// Optional: Set the timeout (in seconds) of the builder. When unset, no timeout is | ||
| /// configured and a request can block indefinitely if the server stops responding. | ||
| #[uniffi::constructor(default(proxy = None, timeout = None))] | ||
| pub fn new(url: String, proxy: Option<String>, timeout: Option<u8>) -> Self { |
There was a problem hiding this comment.
Let's mirror the upstream type here and expose a u64 instead of a u8.
There was a problem hiding this comment.
Thanks for explaining the reasoning, and i agree on mirroring upstream, so I just exposed it as u64.
| if let Some(proxy) = proxy { | ||
| builder = builder.proxy(proxy.as_str()); | ||
| } | ||
| if let Some(timeout) = timeout { |
There was a problem hiding this comment.
Given my comment above, the .into() here will become redundant.
There was a problem hiding this comment.
Yeah, I just removed .into() since the parameter is now u64.
EsploraClient::new had no way to set a request timeout: it only forwarded proxy to esplora_client::Builder, leaving the builder's timeout at None. A server that accepts a connection then stops responding could block the calling thread indefinitely. Add an optional timeout, as Option<u64> seconds to match esplora_client::Builder::timeout. It defaults to None so callers opt in and existing behavior is unchanged, and the docs note that a request can block indefinitely when it is unset.
853a5ba to
091c7b2
Compare
This branch has not been deployed
3 participants
Description
Closes #1109
EsploraClient::newhad no way to set a request timeout: proxy was theonly setting it applied to
esplora_client::Builder, leaving thebuilder's timeout at
None. A server that accepts a connection then stopsresponding could block the calling thread indefinitely.
This adds an optional
timeout(seconds), mirroringElectrumClient::new. Itdefaults to
None, so callers opt in.Notes to the reviewers
Noneas suggested, so nothing changes for existing callers.Added a docstring note for the hang risk when it's unset.
Option<u8>seconds matchesElectrumClient.replies. With
timeoutunset the call never returned; withtimeout = 3itraised
Minreq("the timeout of the request was reached")after 3.01s —catchable rather than a hang.
changelog: added?Documentation
esplora_client::Builder::timeoutChecklists
All Submissions:
cargo fmtandcargo clippybefore committingchangelog:*labelNew Features: