From 777fab8c6a202d7682c3e702e4fb87e974092b29 Mon Sep 17 00:00:00 2001 From: Dave Cramer Date: Tue, 8 Nov 2022 10:57:21 -0500 Subject: [PATCH 1/5] Simplify code using try with resources for the Reentrant lock --- .../amazon/jdbc/ConnectionPluginManager.java | 14 +-- .../amazon/jdbc/util/ResourceLock.java | 39 +++++++ .../amazon/jdbc/util/WrapperUtils.java | 11 +- .../amazon/jdbc/util/ResourceLockTest.java | 101 ++++++++++++++++++ 4 files changed, 146 insertions(+), 19 deletions(-) create mode 100644 wrapper/src/main/java/software/amazon/jdbc/util/ResourceLock.java create mode 100644 wrapper/src/test/java/software/amazon/jdbc/util/ResourceLockTest.java diff --git a/wrapper/src/main/java/software/amazon/jdbc/ConnectionPluginManager.java b/wrapper/src/main/java/software/amazon/jdbc/ConnectionPluginManager.java index 2c5654c7e..44fe53ef5 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/ConnectionPluginManager.java +++ b/wrapper/src/main/java/software/amazon/jdbc/ConnectionPluginManager.java @@ -41,10 +41,7 @@ import software.amazon.jdbc.plugin.failover.FailoverConnectionPluginFactory; import software.amazon.jdbc.plugin.staledns.AuroraStaleDnsPluginFactory; import software.amazon.jdbc.profile.DriverConfigurationProfiles; -import software.amazon.jdbc.util.Messages; -import software.amazon.jdbc.util.SqlState; -import software.amazon.jdbc.util.StringUtils; -import software.amazon.jdbc.util.WrapperUtils; +import software.amazon.jdbc.util.*; import software.amazon.jdbc.wrapper.ConnectionWrapper; /** @@ -78,7 +75,7 @@ public class ConnectionPluginManager implements CanReleaseResources { private static final String INIT_HOST_PROVIDER_METHOD = "initHostProvider"; private static final String NOTIFY_CONNECTION_CHANGED_METHOD = "notifyConnectionChanged"; private static final String NOTIFY_NODE_LIST_CHANGED_METHOD = "notifyNodeListChanged"; - private final ReentrantLock lock = new ReentrantLock(); + private final ResourceLock lock = new ResourceLock(); protected Properties props = new Properties(); protected ArrayList plugins; @@ -105,13 +102,10 @@ public ConnectionPluginManager(ConnectionProvider connectionProvider, Connection this.connectionWrapper = connectionWrapper; } - public void lock() { - lock.lock(); + public ResourceLock obtain() { + return lock.obtain(); } - public void unlock() { - lock.unlock(); - } /** * Initialize a chain of {@link ConnectionPlugin} using their corresponding {@link diff --git a/wrapper/src/main/java/software/amazon/jdbc/util/ResourceLock.java b/wrapper/src/main/java/software/amazon/jdbc/util/ResourceLock.java new file mode 100644 index 000000000..1e5a23fa0 --- /dev/null +++ b/wrapper/src/main/java/software/amazon/jdbc/util/ResourceLock.java @@ -0,0 +1,39 @@ +/* + * Copyright (c) 2004, PostgreSQL Global Development Group + * See the LICENSE file in the project root for more information. + */ + +package software.amazon.jdbc.util; + +import java.util.concurrent.locks.ReentrantLock; + +/** + * Extends a ReentrantLock for use in try-with-resources block. + * + *

Example use

+ * 
{@code
+ *
+ *   try (ResourceLock ignore = lock.obtain()) {
+ *     // do something while holding the resource lock
+ *   }
+ *
+ * }
+ */ +public final class ResourceLock extends ReentrantLock implements AutoCloseable { + + /** + * Obtain a lock and return the ResourceLock for use in try-with-resources block. + */ + public ResourceLock obtain() { + lock(); + return this; + } + + /** + * Unlock on exit of try-with-resources block. + */ + @Override + public void close() { + this.unlock(); + } +} \ No newline at end of file diff --git a/wrapper/src/main/java/software/amazon/jdbc/util/WrapperUtils.java b/wrapper/src/main/java/software/amazon/jdbc/util/WrapperUtils.java index 7616d752f..7b94c6773 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/util/WrapperUtils.java +++ b/wrapper/src/main/java/software/amazon/jdbc/util/WrapperUtils.java @@ -178,9 +178,8 @@ public static T executeWithPlugins( final JdbcCallable jdbcMethodFunc, Object... jdbcMethodArgs) { - pluginManager.lock(); - try { + try (ResourceLock ignore = pluginManager.obtain()){ Object[] argsCopy = jdbcMethodArgs == null ? null : Arrays.copyOf(jdbcMethodArgs, jdbcMethodArgs.length); @@ -198,9 +197,6 @@ public static T executeWithPlugins( } catch (InstantiationException e) { throw new RuntimeException(e); } - - } finally { - pluginManager.unlock(); } } @@ -214,9 +210,8 @@ public static T executeWithPlugins( Object... jdbcMethodArgs) throws E { - pluginManager.lock(); - try { + try (ResourceLock lock = pluginManager.obtain()){ Object[] argsCopy = jdbcMethodArgs == null ? null : Arrays.copyOf(jdbcMethodArgs, jdbcMethodArgs.length); @@ -230,8 +225,6 @@ public static T executeWithPlugins( throw new RuntimeException(e); } - } finally { - pluginManager.unlock(); } } diff --git a/wrapper/src/test/java/software/amazon/jdbc/util/ResourceLockTest.java b/wrapper/src/test/java/software/amazon/jdbc/util/ResourceLockTest.java new file mode 100644 index 000000000..3b43d6bb2 --- /dev/null +++ b/wrapper/src/test/java/software/amazon/jdbc/util/ResourceLockTest.java @@ -0,0 +1,101 @@ +package software.amazon.jdbc.util; + +import org.junit.jupiter.api.Test; + +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.locks.LockSupport; + +import static org.junit.jupiter.api.Assertions.*; + +class ResourceLockTest { + + @Test + void testObtainClose() { + final ResourceLock lock = new ResourceLock(); + + assertFalse(lock.isLocked()); + assertFalse(lock.isHeldByCurrentThread()); + + try (ResourceLock ignore = lock.obtain()) { + assertTrue(lock.isLocked()); + assertTrue(lock.isHeldByCurrentThread()); + } + + assertFalse(lock.isLocked()); + assertFalse(lock.isHeldByCurrentThread()); + } + + @Test + void testObtainWhenMultiThreadedExpectLinearExecution() throws InterruptedException, ExecutionException { + CallWithResourceLock callWithResourceLock = new CallWithResourceLock(); + + int levelOfConcurrency = 5; + + ExecutorService executorService = Executors.newFixedThreadPool(levelOfConcurrency); + try { + List> callables = new ArrayList<>(); + for (int i = 0; i < levelOfConcurrency; i++) { + callables.add(callWithResourceLock::invoke); + } + + // expect linear execution + List> results = executorService.invokeAll(callables); + + Set preLockSet = new HashSet<>(); + Set postLockSet = new HashSet<>(); + for (Future result : results) { + CounterPair entry = result.get(); + preLockSet.add(entry.preLock); + postLockSet.add(entry.postLock); + } + + assertEquals(levelOfConcurrency, postLockSet.size()); // linear execution inside resource lock block + assertEquals(1, preLockSet.size()); // all threads called invoke before any finish + + } finally { + executorService.shutdown(); + } + } + + static final class CallWithResourceLock { + + // wait enough time to allow concurrent threads to block on the lock + final long waitTime = TimeUnit.MILLISECONDS.toNanos(20); + final ResourceLock lock = new ResourceLock(); + final AtomicInteger counter = new AtomicInteger(); + + /** + * Invoke returning 'pre lock' and 'post lock' counter value. + */ + CounterPair invoke() { + int preLock = counter.get(); + try (ResourceLock ignore = lock.obtain()) { + int postLock = counter.get(); + LockSupport.parkNanos(waitTime); + counter.incrementAndGet(); + return new CounterPair(preLock, postLock); + } + } + } + + static final class CounterPair { + final int preLock; + final int postLock; + + CounterPair(int preLock, int postLock) { + this.preLock = preLock; + this.postLock = postLock; + } + } + +} \ No newline at end of file From 8bae5554c073bf336a8d88f587ae6a8451aeaa48 Mon Sep 17 00:00:00 2001 From: Dave Cramer Date: Tue, 8 Nov 2022 11:09:19 -0500 Subject: [PATCH 2/5] fix tests --- .../java/software/amazon/jdbc/ConnectionPluginManager.java | 6 ++++++ .../java/software/amazon/jdbc/util/WrapperUtilsTest.java | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/wrapper/src/main/java/software/amazon/jdbc/ConnectionPluginManager.java b/wrapper/src/main/java/software/amazon/jdbc/ConnectionPluginManager.java index 44fe53ef5..ade37eee1 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/ConnectionPluginManager.java +++ b/wrapper/src/main/java/software/amazon/jdbc/ConnectionPluginManager.java @@ -106,6 +106,12 @@ public ResourceLock obtain() { return lock.obtain(); } + /* + For testing only + */ + public void closeResourceLock() { + lock.close(); + } /** * Initialize a chain of {@link ConnectionPlugin} using their corresponding {@link diff --git a/wrapper/src/test/java/software/amazon/jdbc/util/WrapperUtilsTest.java b/wrapper/src/test/java/software/amazon/jdbc/util/WrapperUtilsTest.java index 814bdaae3..b6e4fc151 100644 --- a/wrapper/src/test/java/software/amazon/jdbc/util/WrapperUtilsTest.java +++ b/wrapper/src/test/java/software/amazon/jdbc/util/WrapperUtilsTest.java @@ -49,11 +49,11 @@ void init() { doAnswer(invocation -> { pluginManagerLock.lock(); return null; - }).when(pluginManager).lock(); + }).when(pluginManager).obtain(); doAnswer(invocation -> { pluginManagerLock.unlock(); return null; - }).when(pluginManager).unlock(); + }).when(pluginManager).closeResourceLock(); doAnswer(invocation -> { boolean lockIsFree = testLock.tryLock(); From 2e44870439d73f3f838cd4a9104c4dd6f15c7b06 Mon Sep 17 00:00:00 2001 From: Dave Cramer Date: Tue, 8 Nov 2022 14:42:32 -0500 Subject: [PATCH 3/5] rename functions --- .../java/software/amazon/jdbc/ConnectionPluginManager.java | 5 ++--- .../main/java/software/amazon/jdbc/util/WrapperUtils.java | 4 ++-- .../java/software/amazon/jdbc/util/WrapperUtilsTest.java | 4 ++-- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/wrapper/src/main/java/software/amazon/jdbc/ConnectionPluginManager.java b/wrapper/src/main/java/software/amazon/jdbc/ConnectionPluginManager.java index ade37eee1..5d6b1e4dc 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/ConnectionPluginManager.java +++ b/wrapper/src/main/java/software/amazon/jdbc/ConnectionPluginManager.java @@ -25,7 +25,6 @@ import java.util.Map; import java.util.Properties; import java.util.Set; -import java.util.concurrent.locks.ReentrantLock; import java.util.logging.Logger; import org.checkerframework.checker.nullness.qual.NonNull; import org.checkerframework.checker.nullness.qual.Nullable; @@ -102,14 +101,14 @@ public ConnectionPluginManager(ConnectionProvider connectionProvider, Connection this.connectionWrapper = connectionWrapper; } - public ResourceLock obtain() { + public ResourceLock acquireLock() { return lock.obtain(); } /* For testing only */ - public void closeResourceLock() { + public void releaseLock() { lock.close(); } diff --git a/wrapper/src/main/java/software/amazon/jdbc/util/WrapperUtils.java b/wrapper/src/main/java/software/amazon/jdbc/util/WrapperUtils.java index 7b94c6773..63fbe10f8 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/util/WrapperUtils.java +++ b/wrapper/src/main/java/software/amazon/jdbc/util/WrapperUtils.java @@ -179,7 +179,7 @@ public static T executeWithPlugins( Object... jdbcMethodArgs) { - try (ResourceLock ignore = pluginManager.obtain()){ + try (ResourceLock ignore = pluginManager.acquireLock()){ Object[] argsCopy = jdbcMethodArgs == null ? null : Arrays.copyOf(jdbcMethodArgs, jdbcMethodArgs.length); @@ -211,7 +211,7 @@ public static T executeWithPlugins( throws E { - try (ResourceLock lock = pluginManager.obtain()){ + try (ResourceLock lock = pluginManager.acquireLock()){ Object[] argsCopy = jdbcMethodArgs == null ? null : Arrays.copyOf(jdbcMethodArgs, jdbcMethodArgs.length); diff --git a/wrapper/src/test/java/software/amazon/jdbc/util/WrapperUtilsTest.java b/wrapper/src/test/java/software/amazon/jdbc/util/WrapperUtilsTest.java index b6e4fc151..e7949e12a 100644 --- a/wrapper/src/test/java/software/amazon/jdbc/util/WrapperUtilsTest.java +++ b/wrapper/src/test/java/software/amazon/jdbc/util/WrapperUtilsTest.java @@ -49,11 +49,11 @@ void init() { doAnswer(invocation -> { pluginManagerLock.lock(); return null; - }).when(pluginManager).obtain(); + }).when(pluginManager).acquireLock(); doAnswer(invocation -> { pluginManagerLock.unlock(); return null; - }).when(pluginManager).closeResourceLock(); + }).when(pluginManager).releaseLock(); doAnswer(invocation -> { boolean lockIsFree = testLock.tryLock(); From 0bb39d80ac8401b6543132ee0e710a31356200e0 Mon Sep 17 00:00:00 2001 From: Dave Cramer Date: Thu, 10 Nov 2022 06:59:40 -0500 Subject: [PATCH 4/5] appease checkstyle --- .../amazon/jdbc/ConnectionPluginManager.java | 6 ++- .../amazon/jdbc/util/ResourceLock.java | 44 ++++++++++++------- .../amazon/jdbc/util/WrapperUtils.java | 4 +- 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/wrapper/src/main/java/software/amazon/jdbc/ConnectionPluginManager.java b/wrapper/src/main/java/software/amazon/jdbc/ConnectionPluginManager.java index 5d6b1e4dc..6a42a672c 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/ConnectionPluginManager.java +++ b/wrapper/src/main/java/software/amazon/jdbc/ConnectionPluginManager.java @@ -40,7 +40,11 @@ import software.amazon.jdbc.plugin.failover.FailoverConnectionPluginFactory; import software.amazon.jdbc.plugin.staledns.AuroraStaleDnsPluginFactory; import software.amazon.jdbc.profile.DriverConfigurationProfiles; -import software.amazon.jdbc.util.*; +import software.amazon.jdbc.util.Messages; +import software.amazon.jdbc.util.ResourceLock; +import software.amazon.jdbc.util.SqlState; +import software.amazon.jdbc.util.StringUtils; +import software.amazon.jdbc.util.WrapperUtils; import software.amazon.jdbc.wrapper.ConnectionWrapper; /** diff --git a/wrapper/src/main/java/software/amazon/jdbc/util/ResourceLock.java b/wrapper/src/main/java/software/amazon/jdbc/util/ResourceLock.java index 1e5a23fa0..13a0b19ae 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/util/ResourceLock.java +++ b/wrapper/src/main/java/software/amazon/jdbc/util/ResourceLock.java @@ -1,5 +1,19 @@ /* - * Copyright (c) 2004, PostgreSQL Global Development Group + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * 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 + * + * http://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. + * + * portions Copyright (c) 2022, PostgreSQL Global Development Group * See the LICENSE file in the project root for more information. */ @@ -21,19 +35,19 @@ */ public final class ResourceLock extends ReentrantLock implements AutoCloseable { - /** - * Obtain a lock and return the ResourceLock for use in try-with-resources block. - */ - public ResourceLock obtain() { - lock(); - return this; - } + /** + * Obtain a lock and return the ResourceLock for use in try-with-resources block. + */ + public ResourceLock obtain() { + lock(); + return this; + } - /** - * Unlock on exit of try-with-resources block. - */ - @Override - public void close() { - this.unlock(); - } + /** + * Unlock on exit of try-with-resources block. + */ + @Override + public void close() { + this.unlock(); + } } \ No newline at end of file diff --git a/wrapper/src/main/java/software/amazon/jdbc/util/WrapperUtils.java b/wrapper/src/main/java/software/amazon/jdbc/util/WrapperUtils.java index 63fbe10f8..a9f0a6e9d 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/util/WrapperUtils.java +++ b/wrapper/src/main/java/software/amazon/jdbc/util/WrapperUtils.java @@ -179,7 +179,7 @@ public static T executeWithPlugins( Object... jdbcMethodArgs) { - try (ResourceLock ignore = pluginManager.acquireLock()){ + try (ResourceLock ignore = pluginManager.acquireLock()) { Object[] argsCopy = jdbcMethodArgs == null ? null : Arrays.copyOf(jdbcMethodArgs, jdbcMethodArgs.length); @@ -211,7 +211,7 @@ public static T executeWithPlugins( throws E { - try (ResourceLock lock = pluginManager.acquireLock()){ + try (ResourceLock lock = pluginManager.acquireLock()) { Object[] argsCopy = jdbcMethodArgs == null ? null : Arrays.copyOf(jdbcMethodArgs, jdbcMethodArgs.length); From 09e825df9c227bf4ba235f8b978f4cf367c01652 Mon Sep 17 00:00:00 2001 From: Dave Cramer Date: Thu, 10 Nov 2022 07:21:46 -0500 Subject: [PATCH 5/5] we have a rule that states that line 15 must end the comment section. This is probably too specific --- .../src/main/java/software/amazon/jdbc/util/ResourceLock.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/wrapper/src/main/java/software/amazon/jdbc/util/ResourceLock.java b/wrapper/src/main/java/software/amazon/jdbc/util/ResourceLock.java index 13a0b19ae..d5f1f7689 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/util/ResourceLock.java +++ b/wrapper/src/main/java/software/amazon/jdbc/util/ResourceLock.java @@ -12,7 +12,8 @@ * 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. - * + */ +/* * portions Copyright (c) 2022, PostgreSQL Global Development Group * See the LICENSE file in the project root for more information. */