Retry::notify should take an async function instead of a sync function

[cdhowie]

It's a bit odd that Retry::notify takes a sync function when Retry is intended for async usage. We have a case where we need to notify another entity asynchronously. In order to do this, we will have to clone something and spawn a background task.

To be honest, the same probably goes for every other closure-taking method on Retry. I think it makes sense for them all to be async.

[Xuanwo]

Seems reasonable. Any idea how to implement this at zero cost?

[cdhowie]

I'm not sure if it can be strictly zero cost, though it may optimize to be that way when async is not needed.

As far as I can tell, the crucial part of this change is that RetryConfig::decide needs to return a future. It would seem reasonable to allow all 3 functions used here to be async. This will require the introduction of another future type (e.g. Decide) as well as an additional retry state (Deciding) to hold that future.

Obviously, this is a breaking change and would require a new major release.

[cdhowie]

I started working on a proof of concept of this today. I'm hoping I'll have a branch with the implementation later this week.

[cdhowie]

I have a somewhat working implementation (passes all tests except doc tests which I haven't fixed yet) but there are rough edges due to the classic problem of HRTBs not being able to span bounds. I have this, for example, on Retry:

NF: FnMut(&E, Duration) -> NFF,
NFF: Future<Output = ()>,

But since we can't tell the compiler that NFF captures the lifetime of &E, the returned future is not allowed to capture the &E which severely limits its usefulness.

There are several workarounds but each have their own disadvantages.

1. The only way I know of to get this to work on stable Rust would be to use boxed futures, as the lifetime can be expressed in that case (NF: for<'a> FnMut(&'a E, Duration) -> BoxFuture<'a, ()>). It's worth noting that even this approach will add a bit of complexity to the implementation because the lifetime of the future won't be valid due to pinning shenanigans, so the lifetime would have to be transmuted for storage in Retry (which is legal and sound, but must be done carefully). The downside here is obviously requiring allocation for all users, which is not desirable.

2. The "no workaround" workaround -- just leave it as is.

   Some users might be able to grab what they need from the error and then move that into an async move block instead, which is a common pattern in async Rust so this might not be too bad anyway.

   Other users who do need the actual, original error in the future could use a cheaply clonable error type (e.g. Arc<E>) which can then be cloned and the clone captured. This may have the same disadvantage as boxing the future (requiring an allocation) but only in cases where the user needs a custom function and cannot grab the state they need from the error and move it into an async block; the default functions don't use the error so they can just return ready(_).

3. Pass a *const E instead. This solves the problem but requires the user understand how long they can use the error (which is until their function's future returns Ready) as well as requiring them to use unsafe.
   We might be able to use AsyncFnMut at some point, but since its CallRefFuture associated type is unstable, we can't name the future type which creates problems when we want to name the returned future to store it in a state enum.

4. The functions are changed to take E by value and must return an E. This works but is a bit clunky of an API.

I am leaning towards option 2 because it does not require allocation and there is a fairly-idiomatic workaround. However, I think option 4 is also reasonable.

[cdhowie]

I pushed a branch with an implementation that passes all tests. This is not yet ready to be merged (there are some unsafe blocks that need documentation, at a minimum) but I wanted to point out a few things about it if you wanted to take a glance before I work more on it.

• The samples/documentation are all adjusted to return Ready<_> from these functions. Simply changing them to async fns will fail because of the issue discussed above; an async fn will capture the passed references, which will cause compilation to fail. We probably need to add documentation and examples of how to handle this in more complex scenarios.
• The additional generic parameters for future types around Retry and RetryWithContext cause clippy type_complexity warnings. There's not really any way to factor out the type without needing just as many generic arguments, so we may need to allow(clippy::type_complexity) in these cases.
• There is a new future type Decide used by both Retry and RetryWithContext which references the RetryConfig owned by those types. This makes both of those future types self-referential, so they are no longer Unpin.

[Xuanwo]

I don't want to implement it as a breaking change since I don’t want to force everyone to write those functions as async functions or add extra overhead for users who just want to print a log.

If it’s really hard, maybe we can add a new type for retry.

[cdhowie]

I can always rename the methods that take async functions to something else and keep the original methods with the same signature, that will re-wrap the closures they're given in one that returns a ready future.

However, note that it will still be a technically semver-breaking change due to the Retry* futures no longer implementing Unpin.