Shutdown ClusterTopologyRefreshTask properly

[thachlp]

Issue: #2904
Make sure that:

• You have read the contribution guidelines.
• You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
• You applied code formatting rules using the mvn formatter:format target. Don’t submit any formatting related changes.
• You submit test cases (unit or integration tests) that back your changes.

[tishun]

Hey @thachlp,

Thanks for giving this fix a go. I think, however, you may be on the wrong path.

Judging from the stack trace in #2904 the ClusterTopologyRefreshScheduler attempts to refresh the topology AFTER the connections have been closed and the client is shutting down.

The suspendTopologyRefresh() is supposed to suspend any topology refresh tasks, but it seems there is some case (race condition perhaps?) where a task is still executed during shurdown.

[thachlp]

Hey @thachlp,

Thanks for giving this fix a go. I think, however, you may be on the wrong path.

Judging from the stack trace in #2904 the ClusterTopologyRefreshScheduler attempts to refresh the topology AFTER the connections have been closed and the client is shutting down.

The suspendTopologyRefresh() is supposed to suspend any topology refresh tasks, but it seems there is some case (race condition perhaps?) where a task is still executed during shurdown.

From the Java docs of suspendTopologyRefresh

    /**
     * Suspend periodic topology refresh if it was activated previously. Suspending cancels the periodic schedule without
     * interrupting any running topology refresh. Suspension is in place until obtaining a new {@link #connect connection}.
     *
     * @since 6.3
     */
    public void suspendTopologyRefresh() {
        topologyRefreshScheduler.suspendTopologyRefresh();
    }

From my view, when we shut down RedisClusterClient, we should STOP running CANCEL scheduled tasks, that why I write STOP running tasks.

Thank @tishun for explaining to me, do you have any suggestion for the fix?

[tishun]

I will try to come back to you in the end of the week

[mp911de]

This PR introduces a check for a very specific scenario. The change doesn't necessary lead to a proper cancellation as the task itself is comprised from a series of refresh steps that are coupled through completable future's. Specifically, RedisClusterClient.refreshPartitionsAsync(…) is being called that has no notion of being interrupted.

I think conceptually the easiest approach is to synchronize (and wait) until ClusterTopologyRefreshTask has finished before shutting down ClientResources. ClusterTopologyRefreshTask would require a CompletableFuture<Void> that is being completed upon completion of Supplier<CompletionStage<?>>.

It would require also a bit of housekeeping, e.g.

if (isEventLoopActive()) {
    clientResources.eventExecutorGroup().submit(clusterTopologyRefreshTask);
    return true;
}

isn't atomic, EventExecutorGroup.submit(…) could return a failed future that requires consideration as well.

[Kvicii]

@tishun @mp911de @thachlp
Is there any follow-up? I have the same problem.
issue-3089

[tishun]

As @mp911de mentioned we need to devise a better solution to this problem.
He has explained this in his comment here, I also elaborated more in #2904

[thachlp]

As @mp911de mentioned we need to devise a better solution to this problem. He has explained this in his comment here, I also elaborated more in #2904

Thanks @mp911de @tishun for reviews
As I understand, we should add the CompletableFuture to track the canceled completion of ClusterTopologyRefreshTask.
Please help review my implementation 🙇
Btw, is the fail test is random fail, because when I run on local, it was success.

[tishun]

Hey @thachlp ,
apologies, but I think this would still not work. Let me elaborate.

The suspendTopologyRefresh is not the problem itself, because all it does is make sure that no new refresh is scheduled. However any refresh that is already initiated is going to continue anyway.

Then when the event loop is shut down it would print this message.

What we need to do is:

• when we initiate a refresh we need to indicate this with a lock
• at the point where the event loop is being shut down we need to block on the said lock
• if there is no holder of the lock (no refresh currently) the event loop will close normally
• if there is a holder of the lock (a refresh is currently happening) the shutdown would block and wait
• when the refresh is complete we should release the lock (only after the job is complete)

[tishun]

I think we are getting close to what we want to have.
It would be very hard to test this, but if you can think of some unit test that would also be handy. This is good to have, but not mandatory.

Thank you for spending time on this!

[thachlp]

@tishun thanks for reviewing it.
I updated as you commented, please take a look 🙇

[tishun]

Okay, I kind of broke it completely.

I will come back to this next week.