Shutdown ClusterTopologyRefreshTask properly #2985

thachlp · 2024-09-12T06:18:41Z

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.

ggivo · 2024-09-12T10:05:09Z

src/test/java/io/lettuce/core/cluster/RedisClusterClientIntegrationTests.java

+        Delay.delay(Duration.ofMillis(1500));
+        assertThat(clusterClient.isTopologyRefreshInProgress()).isTrue();


Hi, not sure about this.
Topology refresh in test env is quick, and there is no guarantee that we are in right state for the test.
Most likely it will be either not started yet or already completed when assert is performed making the test flaky. Also we are adding a delay to the tests as a hole.

I have run the suggested test and it failed 10 /10 times on the assert.

Do you have any idea to reproduce the issue?

Do you mean how to reproduce the failing test locally or the actual issue?
For the actual issue "java.util.concurrent.RejectedExecutionException", I tried to reproduce it but could not.
I will spend some more time on it next week and see if I can come up with an approach.

Hi @thachlp ,
I took a brief look and I think that this issue can be tested more easily using a unit test. There is a similar one testing client shutdown order already available. You can take a look at here

I suggest using a unit test to confirm that the pending ClusterTopologyRefreshTask is canceled/completed before shutting down the Executor group. We can inject a mock of the ClusterTopologyRefreshTask and complete it after client shutdown is initiated.

Hope it helps

src/main/java/io/lettuce/core/cluster/RedisClusterClient.java

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.

src/main/java/io/lettuce/core/cluster/RedisClusterClient.java

thachlp · 2024-11-04T09:34:37Z

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 · 2024-11-05T12:42:06Z

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

mp911de · 2024-11-06T10:44:33Z

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 · 2024-12-19T04:43:42Z

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

tishun · 2024-12-23T11:03:41Z

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 · 2025-01-02T08:35:54Z

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 · 2025-01-06T17:03:19Z

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!

tishun · 2025-01-31T12:46:11Z

src/main/java/io/lettuce/core/cluster/ClusterTopologyRefreshScheduler.java

+            refreshLock.lock();
+            try {
+                if (compareAndSet(false, true)) {
+                    doRun();
+                    return;
+                }
+
+                if (logger.isDebugEnabled()) {
+                    logger.debug("ClusterTopologyRefreshTask already in progress");
+                }
+            } finally {
+                refreshLock.unlock();


I think we do not need to make any changes here:

the comapreAndSet makes sure there is only one thread that starts a refresh

the suspendTopologyRefresh should stop any new topology refresh attempts

tishun · 2025-01-31T12:48:15Z

src/main/java/io/lettuce/core/cluster/ClusterTopologyRefreshScheduler.java

+                    refreshLock.lock();
+                    try {
+                        set(false);
+                        refreshComplete.signalAll();
+                    } finally {
+                        refreshLock.unlock();
+                    }


I think we need to lock before initiating the topology refresh with reloadTopologyAsync.get() and then unlock after we call set(false)

tishun · 2025-02-01T09:20:14Z

src/main/java/io/lettuce/core/cluster/RedisClusterClient.java

+        ReentrantLock refreshLock = topologyRefreshScheduler.getRefreshLock();
+        Condition refreshComplete = topologyRefreshScheduler.getRefreshComplete();
+
+        refreshLock.lock();
+        try {
+            while (topologyRefreshScheduler.isTopologyRefreshInProgress()) {
+                try {
+                    refreshComplete.await();
+                } catch (InterruptedException e) {
+                    Thread.currentThread().interrupt();
+                }
+            }
+        } finally {
+            refreshLock.unlock();
+        }


Since we know there is only one thread that started a refresh we do not need to wait for more than one lock to be released. We need one lock to acquire (I'd not use a spinlock here, because if a refresh is in progress it might take some time) and then we call the super method, e.g.

refreshLock.lock(); try { return super.shutdownAsync(quietPeriod, timeout, timeUnit); } finally { refreshLock.unlock(); }

ggivo reviewed Sep 12, 2024

View reviewed changes

tishun changed the title ~~Shutdonw clustertopologyrefreshtask properly~~ Shutdown ClusterTopologyRefreshTask properly Sep 13, 2024

tishun requested changes Oct 14, 2024

View reviewed changes

src/main/java/io/lettuce/core/cluster/RedisClusterClient.java Outdated Show resolved Hide resolved

tishun added the status: waiting-for-feedback We need additional information before we can continue label Oct 18, 2024

Karonazaba mentioned this pull request Dec 23, 2024

enablePeriodicRefresh has problematic behavior when service is shut down(close) #3089

Closed

tishun mentioned this pull request Dec 23, 2024

BugReport: ClusterTopologyRefreshTask is not shutdown when RedisClusterClient is shutdown #2904

Open

thachlp closed this Dec 31, 2024

thachlp deleted the shutdonw-clustertopologyrefreshtask-properly branch December 31, 2024 04:14

thachlp restored the shutdonw-clustertopologyrefreshtask-properly branch December 31, 2024 04:16

thachlp reopened this Dec 31, 2024

thachlp added 2 commits December 31, 2024 11:33

Shutdown ClusterTopologyRefreshTask when RedisClusterClient is shutdown

34d1ae6

Update test

d8507a5

thachlp force-pushed the shutdonw-clustertopologyrefreshtask-properly branch from fbb3951 to d8507a5 Compare December 31, 2024 04:34

thachlp added 4 commits December 31, 2024 11:37

Revert change by comment

5952b0b

Add the track of the completion of the topology refresh

ed56fa4

Format

83a470e

Add the track of the completion of the topology refresh cancel

2f38c3c

thachlp requested review from tishun and ggivo January 2, 2025 10:24

Implement lock mechanism

370a172

tishun requested changes Feb 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shutdown ClusterTopologyRefreshTask properly #2985

Shutdown ClusterTopologyRefreshTask properly #2985

thachlp commented Sep 12, 2024 •

edited

Loading

ggivo Sep 12, 2024

thachlp Sep 17, 2024

ggivo Sep 20, 2024

ggivo Sep 23, 2024 •

edited

Loading

tishun left a comment

thachlp commented Nov 4, 2024

tishun commented Nov 5, 2024

mp911de commented Nov 6, 2024

Kvicii commented Dec 19, 2024

tishun commented Dec 23, 2024

thachlp commented Jan 2, 2025

tishun commented Jan 6, 2025

tishun left a comment

tishun Jan 31, 2025

tishun Jan 31, 2025

tishun Feb 1, 2025

		Delay.delay(Duration.ofMillis(1500));
		assertThat(clusterClient.isTopologyRefreshInProgress()).isTrue();

Shutdown ClusterTopologyRefreshTask properly #2985

Are you sure you want to change the base?

Shutdown ClusterTopologyRefreshTask properly #2985

Conversation

thachlp commented Sep 12, 2024 • edited Loading

ggivo Sep 12, 2024

Choose a reason for hiding this comment

thachlp Sep 17, 2024

Choose a reason for hiding this comment

ggivo Sep 20, 2024

Choose a reason for hiding this comment

ggivo Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

tishun left a comment

Choose a reason for hiding this comment

thachlp commented Nov 4, 2024

tishun commented Nov 5, 2024

mp911de commented Nov 6, 2024

Kvicii commented Dec 19, 2024

tishun commented Dec 23, 2024

thachlp commented Jan 2, 2025

tishun commented Jan 6, 2025

tishun left a comment

Choose a reason for hiding this comment

tishun Jan 31, 2025

Choose a reason for hiding this comment

tishun Jan 31, 2025

Choose a reason for hiding this comment

tishun Feb 1, 2025

Choose a reason for hiding this comment

thachlp commented Sep 12, 2024 •

edited

Loading

ggivo Sep 23, 2024 •

edited

Loading