Add Retry utility with RetryPolicy definition #20

G8XSU · 2023-12-02T02:12:47Z

Includes implementation for ExponentialBackoffRetryPolicy and JitteredRetryPolicy

tnull

Thanks, generally LGTM!

While I think it also makes sense to have these utilities live in this crate, I want to note that LDK Node would also probably benefit from having access to them as we already implement (much more simple/crude) retrying in a few places there. Do you have an idea how we could make it happen to have them reusable?

src/error.rs

src/util/retry.rs

tnull · 2023-12-04T10:34:33Z

src/util/retry.rs

+/// A function that performs and retries the given operation, acc. to a retry policy and a set of
+/// retriable errors.
+pub async fn retry<R, F, Fut, T, E>(
+	mut operation: F, retry_policy: &R, retriable_errors: Option<HashSet<E>>,


IIUC, this this could be made a bit more simple by just leaving out the Option?

Yeah but we need the functionality to retry on all errors instead of having to specify all errors.

Ah, makes sense! Could you add a few more words in the fn retry docs that describes the intended usage of this API, i.e., also that the user may specify a set of errors that if only these variants should be retried? Given the discussion around the Hash impl above it might also be good to specify how these errors are matched exactly, i.e., if they really need to be idenentical or not.

Maybe we just pass a Fn? Feels a little more rust idiomatic, at least. Callers would likely just hardcode a few error checks without using a container.

Functional interface for this seems like an overkill.
Callers would normally just retry on fixed set of errors.

I am just worried if clients trip up on it and end up with an infinite loop in production. (and our new api enables that)
Often enough, retry policies don't get properly tested in client code.

Whereas with previous api, client was forced to input some(max_attempts), they could still input none explicitly and get infinite retries.

(a doc warning might help but it is still not as good as interface.)

one way would be to remove with_max_attempts from RetryPolicy and have ExponentialBackoffRetryPolicy
like

pub fn new<E: Error>(base_delay: Duration, max_attempts: Option<u32>) -> MaxAttemptsRetryPolicy<Self, E> { MaxAttemptsRetryPolicy { inner_policy: ExponentialBackoffRetryPolicy { base_delay }, max_attempts, phantom: PhantomData } }

But it is not ideal because if we have to introduce another decorator in constructor, it is statically linked to MaxAttemptsRetryPolicy.

You could have the retry interface take a MaxAttemptsRetryPolicy instead of a generic RetryPolicy if you want to enforce it at the interface level, I suppose.

Option-1 Remove with_max_attempts and MaxAttempts decorator, have this implementation inside each concrete policy such as ExponentialBackoff. take max_attempts as arg in constructor.
Option-2 Keep interface as it is now, using with_max_attempts. (Ack risk that client could misuse it)

Hmm...

Option 3: Change retry to take &MaxAttemptsRetryPolicy<R> instead of &R.

Also, could add a without_attempt_limit provided function to RetryPolicy returning a MaxAttemptsRetryPolicy, changing the max_attempts field to an Option where it is set to None. That way, you can still enforce making a choice even if the user doesn't want a limit.

Change retry to take &MaxAttemptsRetryPolicy instead of &R.

This is limiting, it forces MaxAttempts to be the last decorator that is applied.

could add a without_attempt_limit provided function to RetryPolicy returning a MaxAttemptsRetryPolicy, changing the max_attempts field to an Option where it is set to None.

I am not sure how this would work, in current impl, if we use the same decorator twice, both of them are applied. (previous one doesn't get overridden)

G8XSU · 2023-12-04T20:03:34Z

I want to note that LDK Node would also probably benefit from having access to them as we already implement (much more simple/crude) retrying in a few places there. Do you have an idea how we could make it happen to have them reusable?

It is pub and exposed from this crate. (But I am not sure if we would want to use them as it is in ldk-node, but maybe we can since it is internal repo?)
I also thought of having something similar in rust-lightning. but in vss-client we can't use any of that.

src/util/retry.rs

tnull

LGTM, mod outstanding feedback (drop Hash impl and use Vec, add a sentence more to explain usage of retriable_errors).

jkczyz · 2023-12-06T19:18:46Z

src/util/retry.rs

+/// A function that performs and retries the given operation, acc. to a retry policy and a set of
+/// retriable errors.
+pub async fn retry<R, F, Fut, T, E>(
+	mut operation: F, retry_policy: &R, retriable_errors: Option<HashSet<E>>,


Maybe we just pass a Fn? Feels a little more rust idiomatic, at least. Callers would likely just hardcode a few error checks without using a container.

src/util/retry.rs

G8XSU · 2023-12-07T18:32:20Z

@jkczyz @tnull
I am still bit divided b/w (decorator & functional-interface) and simple functions approach(without 6c824df).
My main concern is trouble for bindings if we need them later on. Fn/closure parameter for error discriminator and decorator functions such as with_* in RetryPolicy will work fine in rust but might have problems in other languages.

(I wouldn't want to do some separate thing if that's too complicated, just to make bindings work)

Would also like to hear from @tnull on this.

jkczyz · 2023-12-07T19:38:22Z

@jkczyz @tnull I am still bit divided b/w (decorator & functional-interface) and simple functions approach(without 6c824df). My main concern is trouble for bindings if we need them later on. Fn/closure parameter for error discriminator and decorator functions such as with_* in RetryPolicy will work fine in rust but might have problems in other languages.

(I wouldn't want to do some separate thing if that's too complicated, just to make bindings work)

Would also like to hear from @tnull on this.

If the use case is to use this in LDK Node, the user could give the base RetryPolicy to the Node builder and we could provide individual Node builder methods for adding the decorators for bindings users. Thus, the decorator API would only need to be used internally. But rust users could still use the decorators if preferred.

tnull · 2023-12-08T09:02:31Z

@jkczyz @tnull I am still bit divided b/w (decorator & functional-interface) and simple functions approach(without 6c824df). My main concern is trouble for bindings if we need them later on. Fn/closure parameter for error discriminator and decorator functions such as with_* in RetryPolicy will work fine in rust but might have problems in other languages.

(I wouldn't want to do some separate thing if that's too complicated, just to make bindings work)

Would also like to hear from @tnull on this.

I think I have no strong opinion which way to go. Decorators can be nice in Rust, but also happy to go another way.

Regarding bindings compatibility:

I don't expect this to be exposed in the LDK Node API somewhere, but just be set to a certain RetryPolicy and used internally, so exposing it via LDK Node bindings shouldn't become an issue.
It should be noted however that Uniffi bindings don't support generics, i.e., we really can't let the type parameters bubble up to, say, the Node object. For context, this is already an issue for KVStore where we didn't find a good alternative and currently just 'hard code' SqliteStore via the LDKNode type alias. Note that this will mean for VSS we'll have to do the same, i.e., create a distinct LDKNodeWithVSS alias that, as far as bindings are concerned, will be an entirely separate object (with copy/pasted API definitions, that will need to be maintained every time we change something..).
This also has to be kept in mind if the plan would be to use Uniffi for VssClient. However, so far my understanding was that it would be exposed via LDK's generator, which should be fine?

So TLDR: no strong opinion, shouldn't be an issue for LDK Node as long as it's not exposed in API and there is no requirement to bubble up the generics.

G8XSU · 2023-12-13T00:18:44Z

In addition to ldk-node,
I was also considering if we ever need to create bindings for vss-client itself in future.

Where certain parts of it are easy to generate bindings such as retry-helper, but if i complicate this with use of above mentioned features such as functional interface and decorator-builder pattern, then it might be difficult to expose them as it is.

jkczyz · 2023-12-13T14:54:46Z

In addition to ldk-node, I was also considering if we ever need to create bindings for vss-client itself in future.

Where certain parts of it are easy to generate bindings such as retry-helper, but if i complicate this with use of above mentioned features such as functional interface and decorator-builder pattern, then it might be difficult to expose them as it is.

Mentioned earlier, but you can always expose a different interface for bindings that uses this interface internally. We shouldn't necessarily let bindings restrictions affect how we define our abstractions.

jkczyz

Overall looks good!

src/util/retry.rs

src/error.rs

src/util/mod.rs

G8XSU · 2023-12-13T20:37:38Z

@jkczyz Thanks for the review.
Yes some docs and stuff need cleaning up after the latest changes,
But looking to get alignment on broader stuff first, mainly this for now: #20 (comment)

jkczyz

Code itself LGTM. Comments primarily on docs and tests.

src/util/retry.rs

jkczyz · 2023-12-15T22:04:33Z

src/util/retry.rs

+/// # 		let max_attempts = 3;
+/// # 		let max_total_delay = Duration::from_secs(60);
+/// # 		let max_jitter = Duration::from_millis(5);


I think it would be better to inline these since they are just repeated in the method names.

they don't repeat in docs, since in definition is hidden from user. (using #).

I prefer this and find it more readable than in-line.

I mean that the method and variable names are essentially the same. It would better to inline the values because the example would then demonstrate the types that the methods take.

src/util/retry.rs

tests/retry_tests.rs

src/util/mod.rs

tnull

LGTM, feel free to land as is.

tnull · 2023-12-19T10:37:26Z

src/util/retry.rs

@@ -20,7 +20,8 @@ use std::time::Duration;
 /// # }
 /// #
 /// let retry_policy = ExponentialBackoffRetryPolicy::new(Duration::from_millis(100))
-/// 	.with_max_attempts(5);
+/// 	.with_max_attempts(5)
+///   .with_max_total_delay(Duration::from_secs(2));


nit: Could be indented one space more to fix alignment.

since it is an intermediate commit and it gets fixed in next commit, will ignore it.

Lol, sorry, error on my part. Should always check if stuff is still present in the final changeset when reviewing commit-by-commit.

G8XSU requested a review from jkczyz December 2, 2023 02:12

tnull reviewed Dec 4, 2023

View reviewed changes

tnull reviewed Dec 5, 2023

View reviewed changes

src/util/retry.rs Outdated Show resolved Hide resolved

src/util/retry.rs Outdated Show resolved Hide resolved

tnull reviewed Dec 5, 2023

View reviewed changes

src/util/retry.rs Outdated Show resolved Hide resolved

tnull reviewed Dec 6, 2023

View reviewed changes

jkczyz reviewed Dec 6, 2023

View reviewed changes

src/util/retry.rs Outdated Show resolved Hide resolved

jkczyz reviewed Dec 7, 2023

View reviewed changes

jkczyz reviewed Dec 13, 2023

View reviewed changes

G8XSU requested a review from jkczyz December 14, 2023 23:40

jkczyz reviewed Dec 15, 2023

View reviewed changes

src/util/mod.rs Outdated Show resolved Hide resolved

G8XSU requested a review from jkczyz December 18, 2023 21:21

G8XSU added 7 commits December 18, 2023 14:29

Add Retry utility with RetryPolicy definition

d00287c

Add ExponentialBackoffRetryPolicy

0b0e5f9

Add MaxAttemptsRetryPolicy

8a0b82d

Add MaxTotalDelayRetryPolicy

444d0c4

Add JitteredRetryPolicy

44fae32

Add FilteredRetryPolicy

e2f29a4

Add Retry and RetryPolicy tests

e7fa784

G8XSU force-pushed the retry branch from 5984592 to e7fa784 Compare December 18, 2023 22:44

jkczyz approved these changes Dec 18, 2023

View reviewed changes

tnull approved these changes Dec 19, 2023

View reviewed changes

G8XSU merged commit c5564e7 into lightningdevkit:main Dec 19, 2023

This was referenced Aug 19, 2025

Enable client-side timeouts #39

Open

Bump MSRV and reqwest dependency #38

Merged

Add Retry utility with RetryPolicy definition #20

Add Retry utility with RetryPolicy definition #20

Uh oh!

Conversation

G8XSU commented Dec 2, 2023

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G8XSU Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G8XSU commented Dec 4, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

G8XSU commented Dec 7, 2023

Uh oh!

jkczyz commented Dec 7, 2023

Uh oh!

tnull commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

G8XSU commented Dec 13, 2023

Uh oh!

jkczyz commented Dec 13, 2023

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

G8XSU Dec 13, 2023 •

edited

Loading

tnull commented Dec 8, 2023 •

edited

Loading

tnull Dec 19, 2023 •

edited

Loading