Skip to content

Commit e1b4b90

Browse files
stIncMalestIncMale
authored
Use Lock.lockInterruptibly only where it may actually be needed (#1220)
JAVA-5189
1 parent ffe8403 commit e1b4b90

14 files changed

+121
-97
lines changed

‎driver-core/src/main/com/mongodb/KerberosSubjectProvider.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
import java.util.concurrent.locks.ReentrantLock;
3030

3131
import static com.mongodb.assertions.Assertions.notNull;
32-
import static com.mongodb.internal.Locks.checkedWithLock;
32+
import static com.mongodb.internal.Locks.checkedWithInterruptibleLock;
3333
import static java.lang.String.format;
3434
import static java.util.concurrent.TimeUnit.MILLISECONDS;
3535
import static java.util.concurrent.TimeUnit.MINUTES;
@@ -91,7 +91,7 @@ private KerberosSubjectProvider(final String loginContextName, @Nullable final S
9191
*/
9292
@NonNull
9393
public Subject getSubject() throws LoginException {
94-
return checkedWithLock(lock, () -> {
94+
return checkedWithInterruptibleLock(lock, () -> {
9595
if (subject == null || needNewSubject(subject)) {
9696
subject = createNewSubject();
9797
}

‎driver-core/src/main/com/mongodb/connection/netty/NettyStream.java

+1-2
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,6 @@
6868

6969
import static com.mongodb.assertions.Assertions.assertNotNull;
7070
import static com.mongodb.assertions.Assertions.isTrueArgument;
71-
import static com.mongodb.internal.Locks.lockInterruptibly;
7271
import static com.mongodb.internal.Locks.withLock;
7372
import static com.mongodb.internal.connection.SslHelper.enableHostNameVerification;
7473
import static com.mongodb.internal.connection.SslHelper.enableSni;
@@ -288,7 +287,7 @@ public void readAsync(final int numBytes, final AsyncCompletionHandler<ByteBuf>
288287
private void readAsync(final int numBytes, final AsyncCompletionHandler<ByteBuf> handler, final long readTimeoutMillis) {
289288
ByteBuf buffer = null;
290289
Throwable exceptionResult = null;
291-
lockInterruptibly(lock);
290+
lock.lock();
292291
try {
293292
exceptionResult = pendingException;
294293
if (exceptionResult == null) {

‎driver-core/src/main/com/mongodb/internal/Locks.java

+59-7
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,27 @@ public static <V> V withLock(final Lock lock, final Supplier<V> supplier) {
4040
}
4141

4242
public static <V, E extends Exception> V checkedWithLock(final Lock lock, final CheckedSupplier<V, E> supplier) throws E {
43+
lock.lock();
44+
try {
45+
return supplier.get();
46+
} finally {
47+
lock.unlock();
48+
}
49+
}
50+
51+
public static void withInterruptibleLock(final Lock lock, final Runnable action) throws MongoInterruptedException {
52+
withInterruptibleLock(lock, () -> {
53+
action.run();
54+
return null;
55+
});
56+
}
57+
58+
public static <V> V withInterruptibleLock(final Lock lock, final Supplier<V> supplier) throws MongoInterruptedException {
59+
return checkedWithInterruptibleLock(lock, supplier::get);
60+
}
61+
62+
public static <V, E extends Exception> V checkedWithInterruptibleLock(final Lock lock, final CheckedSupplier<V, E> supplier)
63+
throws MongoInterruptedException, E {
4364
lockInterruptibly(lock);
4465
try {
4566
return supplier.get();
@@ -56,20 +77,51 @@ public static void lockInterruptibly(final Lock lock) throws MongoInterruptedExc
5677
}
5778
}
5879

80+
/**
81+
* See {@link #lockInterruptiblyUnfair(ReentrantLock)} before using this method.
82+
*/
83+
public static void withUnfairLock(final ReentrantLock lock, final Runnable action) {
84+
withUnfairLock(lock, () -> {
85+
action.run();
86+
return null;
87+
});
88+
}
89+
90+
/**
91+
* See {@link #lockInterruptiblyUnfair(ReentrantLock)} before using this method.
92+
*/
93+
public static <V> V withUnfairLock(final ReentrantLock lock, final Supplier<V> supplier) {
94+
lockUnfair(lock);
95+
try {
96+
return supplier.get();
97+
} finally {
98+
lock.unlock();
99+
}
100+
}
101+
102+
private static void lockUnfair(
103+
// The type must be `ReentrantLock`, not `Lock`,
104+
// because only `ReentrantLock.tryLock` is documented to have the barging (unfair) behavior.
105+
final ReentrantLock lock) {
106+
if (!lock.tryLock()) {
107+
lock.lock();
108+
}
109+
}
110+
111+
/**
112+
* This method allows a thread to attempt acquiring the {@code lock} unfairly despite the {@code lock}
113+
* being {@linkplain ReentrantLock#ReentrantLock(boolean) fair}. In most cases you should create an unfair lock,
114+
* instead of using this method.
115+
*/
59116
public static void lockInterruptiblyUnfair(
60117
// The type must be `ReentrantLock`, not `Lock`,
61-
// because only `ReentrantLock.tryLock` is documented to have the barging behavior.
118+
// because only `ReentrantLock.tryLock` is documented to have the barging (unfair) behavior.
62119
final ReentrantLock lock) throws MongoInterruptedException {
63120
if (Thread.currentThread().isInterrupted()) {
64121
throw interruptAndCreateMongoInterruptedException(null, null);
65122
}
66-
// `ReentrantLock.tryLock` is unfair
67123
if (!lock.tryLock()) {
68-
try {
69-
lock.lockInterruptibly();
70-
} catch (InterruptedException e) {
71-
throw interruptAndCreateMongoInterruptedException(null, e);
72-
}
124+
lockInterruptibly(lock);
73125
}
74126
}
75127

‎driver-core/src/main/com/mongodb/internal/connection/BaseCluster.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
import com.mongodb.event.ClusterDescriptionChangedEvent;
3030
import com.mongodb.event.ClusterListener;
3131
import com.mongodb.event.ClusterOpeningEvent;
32-
import com.mongodb.internal.Locks;
3332
import com.mongodb.internal.VisibleForTesting;
3433
import com.mongodb.internal.async.SingleResultCallback;
3534
import com.mongodb.internal.diagnostics.logging.Logger;
@@ -56,6 +55,7 @@
5655
import static com.mongodb.connection.ServerDescription.MAX_DRIVER_WIRE_VERSION;
5756
import static com.mongodb.connection.ServerDescription.MIN_DRIVER_SERVER_VERSION;
5857
import static com.mongodb.connection.ServerDescription.MIN_DRIVER_WIRE_VERSION;
58+
import static com.mongodb.internal.Locks.withInterruptibleLock;
5959
import static com.mongodb.internal.VisibleForTesting.AccessModifier.PRIVATE;
6060
import static com.mongodb.internal.connection.EventHelper.wouldDescriptionsGenerateEquivalentEvents;
6161
import static com.mongodb.internal.event.EventListenerHelper.singleClusterListener;
@@ -223,7 +223,7 @@ public ClusterDescription getCurrentDescription() {
223223

224224
@Override
225225
public void withLock(final Runnable action) {
226-
Locks.withLock(lock, action);
226+
withInterruptibleLock(lock, action);
227227
}
228228

229229
private void updatePhase() {

‎driver-core/src/main/com/mongodb/internal/connection/ClusterClock.java

+5-5
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222

2323
import java.util.concurrent.locks.ReentrantLock;
2424

25-
import static com.mongodb.internal.Locks.withLock;
25+
import static com.mongodb.internal.Locks.withInterruptibleLock;
2626

2727
/**
2828
* <p>This class is not part of the public API and may be removed or changed at any time</p>
@@ -33,19 +33,19 @@ public class ClusterClock {
3333
private BsonDocument clusterTime;
3434

3535
public BsonDocument getCurrent() {
36-
return withLock(lock, () -> clusterTime);
36+
return withInterruptibleLock(lock, () -> clusterTime);
3737
}
3838

3939
public BsonTimestamp getClusterTime() {
40-
return withLock(lock, () -> clusterTime != null ? clusterTime.getTimestamp(CLUSTER_TIME_KEY) : null);
40+
return withInterruptibleLock(lock, () -> clusterTime != null ? clusterTime.getTimestamp(CLUSTER_TIME_KEY) : null);
4141
}
4242

4343
public void advance(@Nullable final BsonDocument other) {
44-
withLock(lock, () -> this.clusterTime = greaterOf(other));
44+
withInterruptibleLock(lock, () -> this.clusterTime = greaterOf(other));
4545
}
4646

4747
public BsonDocument greaterOf(@Nullable final BsonDocument other) {
48-
return withLock(lock, () -> {
48+
return withInterruptibleLock(lock, () -> {
4949
if (other == null) {
5050
return clusterTime;
5151
} else if (clusterTime == null) {

‎driver-core/src/main/com/mongodb/internal/connection/ConcurrentPool.java

+12-26
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import static com.mongodb.assertions.Assertions.notNull;
4444
import static com.mongodb.internal.Locks.lockInterruptibly;
4545
import static com.mongodb.internal.Locks.lockInterruptiblyUnfair;
46+
import static com.mongodb.internal.Locks.withUnfairLock;
4647
import static com.mongodb.internal.VisibleForTesting.AccessModifier.PRIVATE;
4748
import static com.mongodb.internal.thread.InterruptionUtil.interruptAndCreateMongoInterruptedException;
4849

@@ -371,8 +372,7 @@ int permits() {
371372
}
372373

373374
boolean acquirePermitImmediateUnfair() {
374-
lockInterruptiblyUnfair(lock);
375-
try {
375+
return withUnfairLock(lock, () -> {
376376
throwIfClosedOrPaused();
377377
if (permits > 0) {
378378
//noinspection NonAtomicOperationOnVolatileField
@@ -381,9 +381,7 @@ boolean acquirePermitImmediateUnfair() {
381381
} else {
382382
return false;
383383
}
384-
} finally {
385-
lock.unlock();
386-
}
384+
});
387385
}
388386

389387
/**
@@ -428,39 +426,30 @@ boolean acquirePermit(final long timeout, final TimeUnit unit) throws MongoInter
428426
}
429427

430428
void releasePermit() {
431-
lockInterruptiblyUnfair(lock);
432-
try {
429+
withUnfairLock(lock, () -> {
433430
assertTrue(permits < maxPermits);
434431
//noinspection NonAtomicOperationOnVolatileField
435432
permits++;
436433
permitAvailableOrClosedOrPausedCondition.signal();
437-
} finally {
438-
lock.unlock();
439-
}
434+
});
440435
}
441436

442437
void pause(final Supplier<MongoException> causeSupplier) {
443-
lockInterruptiblyUnfair(lock);
444-
try {
438+
withUnfairLock(lock, () -> {
445439
if (!paused) {
446440
this.paused = true;
447441
permitAvailableOrClosedOrPausedCondition.signalAll();
448442
}
449443
this.causeSupplier = assertNotNull(causeSupplier);
450-
} finally {
451-
lock.unlock();
452-
}
444+
});
453445
}
454446

455447
void ready() {
456448
if (paused) {
457-
lockInterruptiblyUnfair(lock);
458-
try {
449+
withUnfairLock(lock, () -> {
459450
this.paused = false;
460451
this.causeSupplier = null;
461-
} finally {
462-
lock.unlock();
463-
}
452+
});
464453
}
465454
}
466455

@@ -469,16 +458,14 @@ void ready() {
469458
*/
470459
boolean close() {
471460
if (!closed) {
472-
lockInterruptiblyUnfair(lock);
473-
try {
461+
return withUnfairLock(lock, () -> {
474462
if (!closed) {
475463
closed = true;
476464
permitAvailableOrClosedOrPausedCondition.signalAll();
477465
return true;
478466
}
479-
} finally {
480-
lock.unlock();
481-
}
467+
return false;
468+
});
482469
}
483470
return false;
484471
}
@@ -515,5 +502,4 @@ boolean closed() {
515502
static String sizeToString(final int size) {
516503
return size == INFINITE_SIZE ? "infinite" : Integer.toString(size);
517504
}
518-
519505
}

‎driver-core/src/main/com/mongodb/internal/connection/DefaultConnectionPool.java

+17-25
Original file line numberDiff line numberDiff line change
@@ -98,12 +98,12 @@
9898
import static com.mongodb.assertions.Assertions.isTrue;
9999
import static com.mongodb.assertions.Assertions.notNull;
100100
import static com.mongodb.event.ConnectionClosedEvent.Reason.ERROR;
101+
import static com.mongodb.internal.Locks.lockInterruptibly;
101102
import static com.mongodb.internal.Locks.withLock;
103+
import static com.mongodb.internal.Locks.withUnfairLock;
102104
import static com.mongodb.internal.VisibleForTesting.AccessModifier.PRIVATE;
103105
import static com.mongodb.internal.async.ErrorHandlingResultCallback.errorHandlingCallback;
104106
import static com.mongodb.internal.connection.ConcurrentPool.INFINITE_SIZE;
105-
import static com.mongodb.internal.Locks.lockInterruptibly;
106-
import static com.mongodb.internal.Locks.lockInterruptiblyUnfair;
107107
import static com.mongodb.internal.connection.ConcurrentPool.sizeToString;
108108
import static com.mongodb.internal.event.EventListenerHelper.getConnectionPoolListener;
109109
import static com.mongodb.internal.logging.LogMessage.Component.CONNECTION;
@@ -1103,14 +1103,11 @@ private PooledConnection acquirePermitOrGetAvailableOpenedConnection(final boole
11031103
}
11041104

11051105
private void releasePermit() {
1106-
lockInterruptiblyUnfair(lock);
1107-
try {
1106+
withUnfairLock(lock, () -> {
11081107
assertTrue(permits < maxPermits);
11091108
permits++;
11101109
permitAvailableOrHandedOverOrClosedOrPausedCondition.signal();
1111-
} finally {
1112-
lock.unlock();
1113-
}
1110+
});
11141111
}
11151112

11161113
private void expressDesireToGetAvailableConnection() {
@@ -1141,29 +1138,24 @@ private void giveUpOnTryingToGetAvailableConnection() {
11411138
* from threads that are waiting for a permit to open a connection.
11421139
*/
11431140
void tryHandOverOrRelease(final UsageTrackingInternalConnection openConnection) {
1144-
lockInterruptiblyUnfair(lock);
1145-
try {
1141+
boolean handedOver = withUnfairLock(lock, () -> {
11461142
for (//iterate from first (head) to last (tail)
11471143
MutableReference<PooledConnection> desiredConnectionSlot : desiredConnectionSlots) {
11481144
if (desiredConnectionSlot.reference == null) {
11491145
desiredConnectionSlot.reference = new PooledConnection(openConnection);
11501146
permitAvailableOrHandedOverOrClosedOrPausedCondition.signal();
1151-
return;
1147+
return true;
11521148
}
11531149
}
1154-
} finally {
1155-
lock.unlock();
1150+
return false;
1151+
});
1152+
if (!handedOver) {
1153+
pool.release(openConnection);
11561154
}
1157-
pool.release(openConnection);
11581155
}
11591156

11601157
void signalClosedOrPaused() {
1161-
lockInterruptiblyUnfair(lock);
1162-
try {
1163-
permitAvailableOrHandedOverOrClosedOrPausedCondition.signalAll();
1164-
} finally {
1165-
lock.unlock();
1166-
}
1158+
withUnfairLock(lock, permitAvailableOrHandedOverOrClosedOrPausedCondition::signalAll);
11671159
}
11681160

11691161
/**
@@ -1327,16 +1319,16 @@ private static class AsyncWorkManager implements AutoCloseable {
13271319
}
13281320

13291321
void enqueue(final Task task) {
1330-
lockInterruptibly(lock);
1331-
try {
1322+
boolean closed = withLock(lock, () -> {
13321323
if (initUnlessClosed()) {
13331324
tasks.add(task);
1334-
return;
1325+
return false;
13351326
}
1336-
} finally {
1337-
lock.unlock();
1327+
return true;
1328+
});
1329+
if (closed) {
1330+
task.failAsClosed();
13381331
}
1339-
task.failAsClosed();
13401332
}
13411333

13421334
/**

‎driver-core/src/main/com/mongodb/internal/connection/DefaultServerMonitor.java

+2-7
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@
5252
import static com.mongodb.assertions.Assertions.assertNotNull;
5353
import static com.mongodb.assertions.Assertions.notNull;
5454
import static com.mongodb.connection.ServerType.UNKNOWN;
55-
import static com.mongodb.internal.Locks.lockInterruptibly;
55+
import static com.mongodb.internal.Locks.checkedWithLock;
5656
import static com.mongodb.internal.Locks.withLock;
5757
import static com.mongodb.internal.connection.CommandHelper.HELLO;
5858
import static com.mongodb.internal.connection.CommandHelper.LEGACY_HELLO;
@@ -294,12 +294,7 @@ private void waitForNext() throws InterruptedException {
294294
}
295295

296296
private long waitForSignalOrTimeout() throws InterruptedException {
297-
lockInterruptibly(lock);
298-
try {
299-
return condition.awaitNanos(serverSettings.getHeartbeatFrequency(NANOSECONDS));
300-
} finally {
301-
lock.unlock();
302-
}
297+
return checkedWithLock(lock, () -> condition.awaitNanos(serverSettings.getHeartbeatFrequency(NANOSECONDS)));
303298
}
304299

305300
public void cancelCurrentCheck() {

0 commit comments

Comments
 (0)