Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

An idea for async retry #176

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ashamukov
Copy link
Contributor

@ashamukov ashamukov commented May 18, 2019

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:
async-diagram-1

Checklist:

  • solution for inter-thread retry-context propagation:
    • 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

#154

dsyer and others added 3 commits September 6, 2019 10:46
If a RetryCallback returns an async type it can be wrapped by the
RetryTemplate. The template needs to know about all the callback
return types that need wrapping, via its RetryResultProcessors
property (not set by default). See AsyncRetryTemplateTests for
examples of how to do this.
@dsyer
Copy link
Member

dsyer commented Sep 6, 2019

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

@dsyer
Copy link
Member

dsyer commented Sep 6, 2019

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
Copy link
Contributor Author

@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: ashamukov#2 What do you think?

@artembilan
Copy link
Member

I give that solution 👍

@ashamukov
Copy link
Contributor Author

ashamukov commented Oct 25, 2019

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
Copy link
Member

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
Copy link
Contributor Author

Do you have something to take a look already?

This is an attempt ;) ashamukov/spring-security#1

@artembilan
Copy link
Member

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
Copy link
Contributor Author

Thanks, Artem!

Let's raise an issue

The issue: spring-projects/spring-framework#23876

@dsyer
Copy link
Member

dsyer commented Apr 8, 2020

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants