Remove a unnecessary Thread.sleep() from ClusterCommandExecutor.collectResults()

[dongmyo]

https://github.com/spring-projects/spring-data-redis/blob/main/src/main/java/org/springframework/data/redis/connection/ClusterCommandExecutor.java#L224

when for loop ends with all futures done or cancelled in ClusterCommandExecutor.collectResults().

https://github.com/spring-projects/spring-data-redis/blob/main/src/main/java/org/springframework/data/redis/connection/ClusterCommandExecutor.java#L258

after that, there is no need for thread to be slept, but Thread.sleep() is called unnecessarily.

it could be better to check if the done is false.

try {
    if (!done) {
        Thread.sleep(10);
    }
} catch (InterruptedException e) {
    // ...   
}

[jxblum]

Hi @dongmyo -

I appreciate you raising this issue; nice catch!

I also think you are generally correct, here. However, I have been giving this some serious thought, off-and-on, for the past few days, while working through other SD Redis Issues in our backlog.

In summary, I am thinking this Thread.sleep() call should be removed altogether as I am not entirely convinced that 1) it is strictly necessary, and 2) could be achieved in another, more productive way (such as using Future.get(timeout, :TimeUnit), here). I need to carefully think this through.

I think the code will need to be restructured, which should help simplify the code a bit since it is rather complex to reason about anyway, IMO. It also does not appropriately handle the CancellationException that can be thrown from Future.get(), which is not documented (nor here) either, where the collectResults(..) method is invoked (i.e. from methods that are part of the public API).

In general, arbitrarily calling Thread.sleep(..) is dangerous for at least 2 reasons:

1. Threads in Java are native to the platform on which the JRE and Java application are built and run (unlike the Green Threads, or Project Loom, to an extent, which is now on the horizon). Therefore, Java Threads are subject to the vagaries of scheduling and behavior consistent with the underlying OS and Thread model in use.

2. Secondly, arbitrary Thread.sleep(..) timeouts (e.g. 10 milliseconds +/-) might be perfectly acceptable in some applications while detrimental in others. What is too much or too little? And with Redis being all about efficient/fast access to data, this sort of induced latency is questionable at best.

My plan of attack is to write some tests for this area of code first so that I can then begin to make (structural) changes that ultimately get rid of the Thread.sleep(..) altogether.

I will post back when I have more information to share.

[dongmyo]

@jxblum thank you for your response.
either way, it will be fine as long as unnecessary Thread.sleep() is going to be removed.

[jxblum]

I am finally, now circling back to this.

Knowing significantly more about Spring Data Redis and just Redis in general now, my thinking is this "arbitrary" Thread.sleep() call (here) can be simply removed altogether. It really serves no purpose.

The Future.get() calls (here and here) "waits until the computation completes" anyway.

So, I think it is also unnecessary to use Future.get(timeout, :TimeUnit) in this case given blocking/waiting nature of Future.get() in the first place. Although, it may not be the most effective way of "collecting results" across the nodes in the Redis cluster.

Additionally, after looking through this code, I also think there is a bug!

If execution fails for any node when evaluating the result, then there is no point to re-evaluate the node on a subsequent iteration (in the while loop). That is, I think the futureId should be added to the saveGuard (as so) inside the ExecutionException handler block.

Finally, it is not necessary to "handle" the CancellationException as I previously stated.

[rishiraj88]

Very impressive issue. Thanks.

[jxblum]

Opened PR #2719, which is pending review and merge. Buildable changes captured in issue branch issue/2518.

[jxblum]

Technically, after further review and careful consideration, I should clarify that the Thread.sleep(..) is possibly useful when none of the Future results are complete yet.

Depending on the Future implementation, without a pause, the while loop (here) inside ClusterCommandExecutor.collectResults(..) could run hot and wind up being a "busy" loop.

The new implementation is here, along with this.

I also reduced the Thread.sleep(..) to 1 millisecond.

NOTE: I could have chosen to use a TimeUnit.timedWait(..) (Javadoc), thereby reducing the latency even further to MICROSECONDS (e.g. TimeUnit.MICROSECONDS.timedWait(obj, 10)) or even NANOSECONDS, but that requires a synchronized block on an Object monitor (or an object's intrinsic lock). This would limit scalability in the context of Virtual Threads.

[jxblum]

On second thought, recalling earlier thoughts I had, I do think there is a way to remove the Thread.sleep(..) call completely.

I switched PR #2719 to back DRAFT mode for the time being.