From 34db64c832762a734b08718a7d774215a50f440d Mon Sep 17 00:00:00 2001
From: Artem Bilan <abilan@pivotal.io>
Date: Tue, 11 Apr 2017 11:12:58 -0400
Subject: [PATCH] INT-4254: LockRegLInit: Fix unconditional unlock

JIRA: https://jira.spring.io/browse/INT-4254

The `LockRegistryLeaderInitiator#LeaderSelector` unconditional calls
`unlock()` in the `finally` block when the `Lock` might not be locked.

Another problem that `running = true` is set after submitting `LeaderSelector` task.
That might bring the problem that `LeaderSelector` won't be selected because of
`this.running` race condition

* Move `this.running = true` before submitting `LeaderSelector` task
* Move `unlock()` into the `if (this.locked)` condition

**Cherry-pick to 4.3.x**
---
 .../leader/LockRegistryLeaderInitiator.java   |  4 +-
 .../LockRegistryLeaderInitiatorTests.java     | 52 +++++++++++++++++++
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/spring-integration-core/src/main/java/org/springframework/integration/support/leader/LockRegistryLeaderInitiator.java b/spring-integration-core/src/main/java/org/springframework/integration/support/leader/LockRegistryLeaderInitiator.java
index 7119c86fba..d3a0f97c28 100644
--- a/spring-integration-core/src/main/java/org/springframework/integration/support/leader/LockRegistryLeaderInitiator.java
+++ b/spring-integration-core/src/main/java/org/springframework/integration/support/leader/LockRegistryLeaderInitiator.java
@@ -242,8 +242,8 @@ public class LockRegistryLeaderInitiator implements SmartLifecycle, DisposableBe
 		synchronized (this.lifecycleMonitor) {
 			if (!this.running) {
 				this.leaderSelector = new LeaderSelector(buildLeaderPath());
-				this.future = this.executorService.submit(this.leaderSelector);
 				this.running = true;
+				this.future = this.executorService.submit(this.leaderSelector);
 				logger.debug("Started LeaderInitiator");
 			}
 		}
@@ -346,8 +346,8 @@ public class LockRegistryLeaderInitiator implements SmartLifecycle, DisposableBe
 				}
 			}
 			finally {
-				this.lock.unlock();
 				if (this.locked) {
+					this.lock.unlock();
 					// We are stopping, therefore not leading any more
 					handleRevoked();
 				}
diff --git a/spring-integration-core/src/test/java/org/springframework/integration/support/leader/LockRegistryLeaderInitiatorTests.java b/spring-integration-core/src/test/java/org/springframework/integration/support/leader/LockRegistryLeaderInitiatorTests.java
index a5b9307b5d..2de458b4ce 100644
--- a/spring-integration-core/src/test/java/org/springframework/integration/support/leader/LockRegistryLeaderInitiatorTests.java
+++ b/spring-integration-core/src/test/java/org/springframework/integration/support/leader/LockRegistryLeaderInitiatorTests.java
@@ -17,22 +17,31 @@
 package org.springframework.integration.support.leader;
 
 import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.BDDMockito.given;
+import static org.mockito.BDDMockito.willAnswer;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
 
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 
 import org.junit.Before;
 import org.junit.Test;
 
+import org.springframework.beans.DirectFieldAccessor;
+import org.springframework.core.task.SyncTaskExecutor;
+import org.springframework.core.task.support.ExecutorServiceAdapter;
 import org.springframework.integration.leader.Context;
 import org.springframework.integration.leader.DefaultCandidate;
 import org.springframework.integration.leader.event.DefaultLeaderEventPublisher;
@@ -191,6 +200,49 @@ public class LockRegistryLeaderInitiatorTests {
 		second.stop();
 	}
 
+	@Test
+	public void testGracefulLeaderSelectorExit() throws Exception {
+		AtomicReference<Throwable> throwableAtomicReference = new AtomicReference<>();
+
+		LockRegistry registry = mock(LockRegistry.class);
+
+		Lock lock = spy(new ReentrantLock());
+
+		willAnswer(invocation -> {
+			try {
+				return invocation.callRealMethod();
+			}
+			catch (Throwable e) {
+				throwableAtomicReference.set(e);
+				throw e;
+			}
+		})
+				.given(lock)
+				.unlock();
+
+
+		given(registry.obtain(anyString()))
+				.willReturn(lock);
+
+		LockRegistryLeaderInitiator initiator = new LockRegistryLeaderInitiator(registry);
+
+		willAnswer(invocation -> {
+			initiator.stop();
+			return false;
+		})
+				.given(lock)
+				.tryLock(anyLong(), eq(TimeUnit.MILLISECONDS));
+
+		new DirectFieldAccessor(initiator).setPropertyValue("executorService",
+				new ExecutorServiceAdapter(
+						new SyncTaskExecutor()));
+
+		initiator.start();
+
+		Throwable throwable = throwableAtomicReference.get();
+		assertNull(throwable);
+	}
+
 	private static class CountingPublisher implements LeaderEventPublisher {
 
 		private final CountDownLatch granted;