From a79c245cac273af66f155f1547e7ccf2fea0a270 Mon Sep 17 00:00:00 2001 From: Olivier Hubaut Date: Mon, 21 Dec 2020 15:41:46 +0100 Subject: [PATCH] GH-3445: Fix JdbcLock for IllegalMonitorStateEx Fixes https://github.com/spring-projects/spring-integration/issues/3445 The `JdbcLockRegistry` improperly unlock the delegate (a `ReentrantLock`) in the case of `TransientDataAccessException` or a `TransactionTimedOutException` is thrown. As this occurs in a while loop, the next time the delegate is unlocked, it not longer as a owner thread and throws an `IllegalMonitorStateException`. * Move `delegate.unlock()` outside the while loop in the `JdbcLock.unlock()` method * Add `JdbcLockRegistryDelegateTests` for mocked `LockRepository` to add more coverage for `JdbcLockRegistry` functionality **Cherry-pick to 5.4.x, 5.3.x & 5.2.x** (cherry picked from commit 7e665096823a5ae0d54d450d597ffb9987cfc0d3) --- .../jdbc/lock/JdbcLockRegistry.java | 29 ++-- .../lock/JdbcLockRegistryDelegateTests.java | 125 ++++++++++++++++++ 2 files changed, 141 insertions(+), 13 deletions(-) create mode 100644 spring-integration-jdbc/src/test/java/org/springframework/integration/jdbc/lock/JdbcLockRegistryDelegateTests.java diff --git a/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/lock/JdbcLockRegistry.java b/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/lock/JdbcLockRegistry.java index 9bede5f10c..d5aa450142 100644 --- a/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/lock/JdbcLockRegistry.java +++ b/spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/lock/JdbcLockRegistry.java @@ -51,6 +51,7 @@ import org.springframework.util.Assert; * @author Gary Russell * @author Alexandre Strubel * @author Stefan Vassilev + * @author Olivier Hubaut * * @since 4.3 */ @@ -253,21 +254,23 @@ public class JdbcLockRegistry implements ExpirableLockRegistry, RenewableLockReg this.delegate.unlock(); return; } - while (true) { - try { - this.mutex.delete(this.path); - return; - } - catch (TransientDataAccessException | TransactionTimedOutException e) { - // try again - } - catch (Exception e) { - throw new DataAccessResourceFailureException("Failed to release mutex at " + this.path, e); - } - finally { - this.delegate.unlock(); + try { + while (true) { + try { + this.mutex.delete(this.path); + return; + } + catch (TransientDataAccessException | TransactionTimedOutException e) { + // try again + } + catch (Exception e) { + throw new DataAccessResourceFailureException("Failed to release mutex at " + this.path, e); + } } } + finally { + this.delegate.unlock(); + } } @Override diff --git a/spring-integration-jdbc/src/test/java/org/springframework/integration/jdbc/lock/JdbcLockRegistryDelegateTests.java b/spring-integration-jdbc/src/test/java/org/springframework/integration/jdbc/lock/JdbcLockRegistryDelegateTests.java new file mode 100644 index 0000000000..0efebb97f6 --- /dev/null +++ b/spring-integration-jdbc/src/test/java/org/springframework/integration/jdbc/lock/JdbcLockRegistryDelegateTests.java @@ -0,0 +1,125 @@ +/* + * Copyright 2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.integration.jdbc.lock; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.Random; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import org.springframework.dao.TransientDataAccessException; +import org.springframework.integration.test.util.TestUtils; +import org.springframework.transaction.TransactionTimedOutException; + +/** + * @author Olivier Hubaut + * + * @since 5.2.11 + */ +public class JdbcLockRegistryDelegateTests { + + private JdbcLockRegistry registry; + + private LockRepository repository; + + @BeforeEach + public void clear() { + repository = mock(LockRepository.class); + registry = new JdbcLockRegistry(repository); + + when(repository.acquire(anyString())).thenReturn(true); + } + + @Test + public void testLessAmountOfUnlockThanLock() { + final Random random = new Random(); + final int lockCount = random.nextInt(5) + 1; + final int unlockCount = random.nextInt(lockCount); + + final Lock lock = registry.obtain("foo"); + for (int i = 0; i < lockCount; i++) { + lock.tryLock(); + } + for (int i = 0; i < unlockCount; i++) { + lock.unlock(); + } + + assertThat(TestUtils.getPropertyValue(lock, "delegate", ReentrantLock.class).isLocked()).isTrue(); + } + + @Test + public void testSameAmountOfUnlockThanLock() { + final Random random = new Random(); + final int lockCount = random.nextInt(5) + 1; + + final Lock lock = registry.obtain("foo"); + for (int i = 0; i < lockCount; i++) { + lock.tryLock(); + } + for (int i = 0; i < lockCount; i++) { + lock.unlock(); + } + + assertThat(TestUtils.getPropertyValue(lock, "delegate", ReentrantLock.class).isLocked()).isFalse(); + } + + @Test + public void testTransientDataAccessException() { + final Lock lock = registry.obtain("foo"); + lock.tryLock(); + + final AtomicBoolean shouldThrow = new AtomicBoolean(true); + doAnswer(invocation -> { + if (shouldThrow.getAndSet(false)) { + throw mock(TransientDataAccessException.class); + } + return null; + }).when(repository).delete(anyString()); + + lock.unlock(); + + assertThat(TestUtils.getPropertyValue(lock, "delegate", ReentrantLock.class).isLocked()).isFalse(); + } + + @Test + public void testTransactionTimedOutException() { + final Lock lock = registry.obtain("foo"); + lock.tryLock(); + + final AtomicBoolean shouldThrow = new AtomicBoolean(true); + doAnswer(invocation -> { + if (shouldThrow.getAndSet(false)) { + throw mock(TransactionTimedOutException.class); + } + return null; + }).when(repository).delete(anyString()); + + lock.unlock(); + + assertThat(TestUtils.getPropertyValue(lock, "delegate", ReentrantLock.class).isLocked()).isFalse(); + } + +}