An idea for async retry

[ashamukov]

Hello! Here is a draft for the async retry feature with rescheduling. The goal for now is to discuss: is it worth to implement it this way. (javadoc and formatting are expected to be later)

The general idea is described in the README.dm diff, and also is outlined here:

Checklist:

• [x] solution for inter-thread retry-context propagation:
  • [x] is discussed
  • [ ] merged (+ tests)
• [ ] consensus about the value of such a feature is reached (probably, it is worth to use some full-fledged async-programming framework instead, like zio-retry for scala)
• [ ] migration of the repo to java8 is discussed and performed
• [ ] details of java.util.concurrent.Future support are discussed and implemented Copy pasted from the issue:

  Because it has no "thenApply"-like notation, exception handling and retry scheduling for actual job can not be performed by worker right after previous attempt fails, i.e. invocation of "resultFuture.get()" is required to start even the first retry of actual job. That mixes sync and async approaches in a not very beautiful way, so I propose to not specially support java.util.concurrent.Future at all.

• [ ] javadoc and formatting are polished
• [ ] additional tests are written for:
  • [ ] stateful rescheduling
  • [ ] RejectedExecutionException on rescheduler
  • [ ] InterruptedException

Can be done in separate PRs:

• add ListenableFuture support
• support declarative async retry

https://github.com/spring-projects/spring-retry/issues/154

[dsyer]

I rebased your work onto master. See PR: https://github.com/ashamukov/spring-retry/pull/1 (or alternatively reset your own branch to https://github.com/dsyer/spring-retry/tree/ashamukov-async-retry).

[dsyer]

About RetrySynchronizationManager Supported partially. There is no way to implicitly pass the context into the worker thread, because the worker executor,

True, but it's a really common pattern. The way we normally solve it in Spring projects is to provide a wrapper for the executor that has to be supplied by the user for the background state to be propagated. I expect we can do that - if the user provides the right wrapper they can use RetrySynchronization, otherwise not.

[ashamukov]

@dsyer Hello Dave! Thanks for the rebase.

The way we normally solve it in Spring projects is to provide a wrapper for the executor that has to be supplied by the user for the background state to be propagated.

Here is an example of solution: https://github.com/ashamukov/spring-retry/pull/2 What do you think?

[artembilan]

I give that solution 👍

[ashamukov]

I give that solution 👍

Thanks, Artem!) Just to double-check: are you agree, that in this situation, copy-pasting is better than extracting DelegatingContextExecutor to somewhere like spring-core, and parametrizing it with RetryContext + RetrySynchronizationManager#register in spring-retry, and with SecurityContext + SecurityContextHolder#setContext in spring-security?

[artembilan]

Well, doesn't look like it is going to be an easy task to extract some common abstraction and we may end up with generics hell. Although I wouldn't mind to play with that. Do you have something to take a look already?

[ashamukov]

Do you have something to take a look already?

This is an attempt ;) https://github.com/ashamukov/spring-security/pull/1

[artembilan]

You know, looks cool!

Let's raise an issue in https://github.com/spring-projects/spring-framework/issues and page @rwinch for determining what we would like to do with Spring Security after that common abstraction.

I believe there might be some improvements or changes in your solution, but at least my colleagues will know our position about such a propagation abstraction from one thread to another! 😄

[ashamukov]

Thanks, Artem!

Let's raise an issue

The issue: https://github.com/spring-projects/spring-framework/issues/23876

[dsyer]

I rebased and did some tidying up: https://github.com/dsyer/spring-retry/tree/ashamukov-async-retry. We can start from there if anyone is interested in this. It works for CompletableFuture and Future (as least as much as is tested).