Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify code using try with resources for the Reentrant lock #259

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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<ConnectionPlugin> plugins;
Expand All @@ -105,12 +102,15 @@ public ConnectionPluginManager(ConnectionProvider connectionProvider, Connection
this.connectionWrapper = connectionWrapper;
}

public void lock() {
lock.lock();
public ResourceLock obtain() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to reconsider a method name. It's not clear what "obtain" on plugin manager object does. Does it obtain a plugin manager?

return lock.obtain();
}

public void unlock() {
lock.unlock();
/*
For testing only
*/
public void closeResourceLock() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"releaseLock"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not married to any name. releaseLock is fine, and then we can use acquireLock instead of obtain.

lock.close();
}

/**
Expand Down
39 changes: 39 additions & 0 deletions wrapper/src/main/java/software/amazon/jdbc/util/ResourceLock.java
Original file line number Diff line number Diff line change
@@ -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.
*
* <h2>Example use</h2>
* <pre>{@code
*
* try (ResourceLock ignore = lock.obtain()) {
* // do something while holding the resource lock
* }
*
* }</pre>
*/
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();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -178,9 +178,8 @@ public static <T> T executeWithPlugins(
final JdbcCallable<T, RuntimeException> jdbcMethodFunc,
Object... jdbcMethodArgs) {

pluginManager.lock();

try {
try (ResourceLock ignore = pluginManager.obtain()){
Object[] argsCopy =
jdbcMethodArgs == null ? null : Arrays.copyOf(jdbcMethodArgs, jdbcMethodArgs.length);

Expand All @@ -198,9 +197,6 @@ public static <T> T executeWithPlugins(
} catch (InstantiationException e) {
throw new RuntimeException(e);
}

} finally {
pluginManager.unlock();
}
}

Expand All @@ -214,9 +210,8 @@ public static <T, E extends Exception> T executeWithPlugins(
Object... jdbcMethodArgs)
throws E {

pluginManager.lock();

try {
try (ResourceLock lock = pluginManager.obtain()){
Object[] argsCopy =
jdbcMethodArgs == null ? null : Arrays.copyOf(jdbcMethodArgs, jdbcMethodArgs.length);

Expand All @@ -230,8 +225,6 @@ public static <T, E extends Exception> T executeWithPlugins(
throw new RuntimeException(e);
}

} finally {
pluginManager.unlock();
}
}

Expand Down
101 changes: 101 additions & 0 deletions wrapper/src/test/java/software/amazon/jdbc/util/ResourceLockTest.java
Original file line number Diff line number Diff line change
@@ -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<Callable<CounterPair>> callables = new ArrayList<>();
for (int i = 0; i < levelOfConcurrency; i++) {
callables.add(callWithResourceLock::invoke);
}

// expect linear execution
List<Future<CounterPair>> results = executorService.invokeAll(callables);

Set<Integer> preLockSet = new HashSet<>();
Set<Integer> postLockSet = new HashSet<>();
for (Future<CounterPair> 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;
}
}

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down