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

default implementation of @Backoff() should not use Thread.sleep #388

Open
jonnytest1 opened this issue Oct 17, 2023 · 7 comments
Open

default implementation of @Backoff() should not use Thread.sleep #388

jonnytest1 opened this issue Oct 17, 2023 · 7 comments

Comments

@jonnytest1
Copy link

jonnytest1 commented Oct 17, 2023

we used the @retryable annotation in a project recently and added a backoff of like one day to our decently long running job , due to the implementation with Thread.sleep this meant that no other scheduled jobs would trigger , which i tihnk isnt generally an expected behaviour , instead it should

  1. be dcoumented on @Retryableand its backoff property ,@backoff and its delay property (in all caps if need be) that this sleeps the Thread
  2. the default implementation should be
      TimerTask task = new TimerTask() {
        public void run() {
            System.out.println("Task performed on: " + new Date() + "n" +
              "Thread's name: " + Thread.currentThread().getName());
        }
    };
    Timer timer = new Timer("Timer");
    
    long delay = 1000L;
    timer.schedule(task, delay);
}

or another similar nonblocking implemenation

hakimrabet pushed a commit to hakimrabet/spring-retry that referenced this issue May 23, 2024
@hakimrabet
Copy link

I agree with you jonnytest1.
could you please assign this to me,
There is my PR for fixing this issue , If it's not OK please comment and provide more information for fixing it.

hakimrabet pushed a commit to hakimrabet/spring-retry that referenced this issue May 23, 2024
hakimrabet pushed a commit to hakimrabet/spring-retry that referenced this issue May 24, 2024
hakimrabet pushed a commit to hakimrabet/spring-retry that referenced this issue May 24, 2024
@jonnytest1
Copy link
Author

i do not know how to assign issues to someone 🤔

@artembilan
Copy link
Member

i do not know how to assign issues to someone

The issue can be assigned only to Spring team member.
However that should not be a stopper for community contribution.

We will look into your PR eventually, however I'm a bit skeptic with non-blocking by default.
The retry pattern supposed to be transparent for API consumer. So, you just call the method and expect some logic to be done.
The fact that it is advised with a retry must not bother you. And if you indeed configured very long back-off, it is expected to have call stack blocked.
We cannot release that thread since we don't have a result from business method execution.

What you probably what is something like async retry: #176.
There we would schedule a new CompletebleFuture which would be fulfilled eventually without blocking the current thread.
But as you see it fully involves everything to be async: we just cannot go async with a regular blocking method.

@hakimrabet
Copy link

hakimrabet commented May 24, 2024

indeed you are right,
however it's not for reactive project instead for regular project,
main idea of that is to release the caller thread
When scheduler running thread going to sleep the other scheduler waiting for the thread going to be released

@quaff
Copy link
Contributor

quaff commented Aug 21, 2024

Thread.sleep() will unmount virtual thread from its carrier platform thread, you should try virtual thread from JDK 21+.

@artembilan
Copy link
Member

@quaff ,

your sentence does not compile in my head.
Virtual threads are really from Java 21.
And yes, they are unmounted from platform thread.
But the point of Thread.sleep() is still to block the current thread, which is no difference for us if it is virtual or not.

@quaff
Copy link
Contributor

quaff commented Aug 22, 2024

But the point of Thread.sleep() is still to block the current thread, which is no difference for us if it is virtual or not.

That's expected behavior, we need delay here but do not waste CPU resource.
My point is Thread.sleep() is not terrible now.

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

No branches or pull requests

4 participants