spring-projects / spring-retry

2.15k stars 513 forks source link

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

Open jonnytest1 opened 11 months ago

jonnytest1 commented 11 months ago

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 commented 3 months ago

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.

jonnytest1 commented 3 months ago

i do not know how to assign issues to someone 🤔

artembilan commented 3 months ago

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: https://github.com/spring-projects/spring-retry/pull/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 commented 3 months ago

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 commented 3 weeks ago

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

artembilan commented 3 weeks ago

@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 commented 3 weeks ago

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.