Skip to content

Commit 9d7fab7

Browse files
committed
Fix one more race condition in RedisLeaderTests
https://build.spring.io/browse/INT-SI50X-JOB1-56 We can't wait for the latch in the interruptable code flow; we can't have a round-robing election guarantees. * Add `Thread.sleep(LockRegistryLeaderInitiator.this.busyWaitMillis)` to the `LockRegistryLeaderInitiator` when we restart the main task * Remove latches waiting and thread shifting from the `RedisLockRegistryLeaderInitiatorTests` * Use long `busyWaitMillis` for yielding initiator to let the second candidate to be elected **Cherry-pick to 5.0.x** (cherry picked from commit 081d0d1)
1 parent 19fe056 commit 9d7fab7

File tree

2 files changed

+20
-43
lines changed

2 files changed

+20
-43
lines changed

spring-integration-core/src/main/java/org/springframework/integration/support/leader/LockRegistryLeaderInitiator.java

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -408,7 +408,12 @@ else if (acquired) {
408408
if (isRunning()) {
409409
logger.warn("Restarting LeaderSelector for " + this.context + " because of error.", e);
410410
LockRegistryLeaderInitiator.this.future =
411-
LockRegistryLeaderInitiator.this.executorService.submit(this);
411+
LockRegistryLeaderInitiator.this.executorService.submit(
412+
() -> {
413+
// Give it a chance to elect some other leader.
414+
Thread.sleep(LockRegistryLeaderInitiator.this.busyWaitMillis);
415+
return call();
416+
});
412417
}
413418
return null;
414419
}

spring-integration-redis/src/test/java/org/springframework/integration/redis/leader/RedisLockRegistryLeaderInitiatorTests.java

Lines changed: 14 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,9 @@
2323
import java.util.ArrayList;
2424
import java.util.List;
2525
import java.util.concurrent.CountDownLatch;
26-
import java.util.concurrent.Executor;
2726
import java.util.concurrent.Executors;
2827
import java.util.concurrent.TimeUnit;
2928

30-
import org.apache.commons.logging.Log;
31-
import org.apache.commons.logging.LogFactory;
3229
import org.junit.Rule;
3330
import org.junit.Test;
3431

@@ -40,8 +37,8 @@
4037
import org.springframework.integration.redis.util.RedisLockRegistry;
4138
import org.springframework.integration.support.leader.LockRegistryLeaderInitiator;
4239
import org.springframework.integration.test.rule.Log4j2LevelAdjuster;
40+
import org.springframework.integration.test.support.LongRunningIntegrationTest;
4341
import org.springframework.scheduling.concurrent.CustomizableThreadFactory;
44-
import org.springframework.util.ReflectionUtils;
4542

4643
/**
4744
* @author Artem Bilan
@@ -52,7 +49,8 @@
5249
*/
5350
public class RedisLockRegistryLeaderInitiatorTests extends RedisAvailableTests {
5451

55-
private static final Log logger = LogFactory.getLog(RedisLockRegistryLeaderInitiatorTests.class);
52+
@Rule
53+
public LongRunningIntegrationTest longTests = new LongRunningIntegrationTest();
5654

5755
@Rule
5856
public Log4j2LevelAdjuster adjuster =
@@ -105,63 +103,37 @@ public void testDistributedLeaderElection() throws Exception {
105103
CountDownLatch acquireLockFailed1 = new CountDownLatch(1);
106104
CountDownLatch acquireLockFailed2 = new CountDownLatch(1);
107105

108-
Executor latchesExecutor = Executors.newCachedThreadPool();
109-
110-
initiator1.setLeaderEventPublisher(new CountingPublisher(granted1, revoked1, acquireLockFailed1) {
106+
initiator1.setLeaderEventPublisher(new CountingPublisher(granted1, revoked1, acquireLockFailed1));
111107

112-
@Override
113-
public void publishOnRevoked(Object source, Context context, String role) {
114-
latchesExecutor.execute(() -> {
115-
// It's difficult to see round-robin election, so block one initiator until the second is elected.
116-
try {
117-
assertThat(granted2.await(10, TimeUnit.SECONDS), is(true));
118-
}
119-
catch (InterruptedException e) {
120-
ReflectionUtils.rethrowRuntimeException(e);
121-
}
122-
super.publishOnRevoked(source, context, role);
123-
});
108+
initiator2.setLeaderEventPublisher(new CountingPublisher(granted2, revoked2, acquireLockFailed2));
124109

125-
}
126-
127-
});
128-
129-
initiator2.setLeaderEventPublisher(new CountingPublisher(granted2, revoked2, acquireLockFailed2) {
130-
131-
@Override
132-
public void publishOnRevoked(Object source, Context context, String role) {
133-
latchesExecutor.execute(() -> {
134-
try {
135-
// It's difficult to see round-robin election, so block one initiator until the second is elected.
136-
assertThat(granted1.await(10, TimeUnit.SECONDS), is(true));
137-
}
138-
catch (InterruptedException e) {
139-
ReflectionUtils.rethrowRuntimeException(e);
140-
}
141-
super.publishOnRevoked(source, context, role);
142-
});
143-
}
144-
145-
});
110+
// It's hard to see round-robin election, so let's make the yielding initiator to sleep long before restarting
111+
initiator1.setBusyWaitMillis(5000);
146112

147113
initiator1.getContext().yield();
148114

149115
assertThat(revoked1.await(10, TimeUnit.SECONDS), is(true));
116+
assertThat(granted2.await(10, TimeUnit.SECONDS), is(true));
150117

151118
assertThat(initiator2.getContext().isLeader(), is(true));
152119
assertThat(initiator1.getContext().isLeader(), is(false));
153120

121+
initiator1.setBusyWaitMillis(LockRegistryLeaderInitiator.DEFAULT_BUSY_WAIT_TIME);
122+
initiator2.setBusyWaitMillis(5000);
123+
154124
initiator2.getContext().yield();
155125

156126
assertThat(revoked2.await(10, TimeUnit.SECONDS), is(true));
127+
assertThat(granted1.await(10, TimeUnit.SECONDS), is(true));
157128

158129
assertThat(initiator1.getContext().isLeader(), is(true));
159130
assertThat(initiator2.getContext().isLeader(), is(false));
160131

161132
initiator2.stop();
162133

163134
CountDownLatch revoked11 = new CountDownLatch(1);
164-
initiator1.setLeaderEventPublisher(new CountingPublisher(new CountDownLatch(1), revoked11, new CountDownLatch(1)));
135+
initiator1.setLeaderEventPublisher(new CountingPublisher(new CountDownLatch(1), revoked11,
136+
new CountDownLatch(1)));
165137

166138
initiator1.getContext().yield();
167139

0 commit comments

Comments
 (0)