Skip to content

Commit 7ee8e66

Browse files
jhoellerjhoeller
committed
Avoid internal lifecycle synchronization in favor of lifecycle lock
Closes gh-32284
1 parent 34372ee commit 7ee8e66

File tree

2 files changed

+95
-23
lines changed

2 files changed

+95
-23
lines changed

Diff for: ‎spring-jdbc/src/main/java/org/springframework/jdbc/datasource/SingleConnectionDataSource.java

+30-8
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2023 the original author or authors.
2+
* Copyright 2002-2024 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -22,6 +22,8 @@
2222
import java.lang.reflect.Proxy;
2323
import java.sql.Connection;
2424
import java.sql.SQLException;
25+
import java.util.concurrent.locks.Lock;
26+
import java.util.concurrent.locks.ReentrantLock;
2527

2628
import org.springframework.beans.factory.DisposableBean;
2729
import org.springframework.lang.Nullable;
@@ -73,8 +75,8 @@ public class SingleConnectionDataSource extends DriverManagerDataSource
7375
@Nullable
7476
private Connection connection;
7577

76-
/** Synchronization monitor for the shared Connection. */
77-
private final Object connectionMonitor = new Object();
78+
/** Lifecycle lock for the shared Connection. */
79+
private final Lock connectionLock = new ReentrantLock();
7880

7981

8082
/**
@@ -181,7 +183,8 @@ protected Boolean getAutoCommitValue() {
181183

182184
@Override
183185
public Connection getConnection() throws SQLException {
184-
synchronized (this.connectionMonitor) {
186+
this.connectionLock.lock();
187+
try {
185188
if (this.connection == null) {
186189
// No underlying Connection -> lazy init via DriverManager.
187190
initConnection();
@@ -193,6 +196,9 @@ public Connection getConnection() throws SQLException {
193196
}
194197
return this.connection;
195198
}
199+
finally {
200+
this.connectionLock.unlock();
201+
}
196202
}
197203

198204
/**
@@ -216,9 +222,13 @@ public Connection getConnection(String username, String password) throws SQLExce
216222
*/
217223
@Override
218224
public boolean shouldClose(Connection con) {
219-
synchronized (this.connectionMonitor) {
225+
this.connectionLock.lock();
226+
try {
220227
return (con != this.connection && con != this.target);
221228
}
229+
finally {
230+
this.connectionLock.unlock();
231+
}
222232
}
223233

224234
/**
@@ -241,11 +251,15 @@ public void close() {
241251
*/
242252
@Override
243253
public void destroy() {
244-
synchronized (this.connectionMonitor) {
254+
this.connectionLock.lock();
255+
try {
245256
if (this.target != null) {
246257
closeConnection(this.target);
247258
}
248259
}
260+
finally {
261+
this.connectionLock.unlock();
262+
}
249263
}
250264

251265

@@ -256,7 +270,8 @@ public void initConnection() throws SQLException {
256270
if (getUrl() == null) {
257271
throw new IllegalStateException("'url' property is required for lazily initializing a Connection");
258272
}
259-
synchronized (this.connectionMonitor) {
273+
this.connectionLock.lock();
274+
try {
260275
if (this.target != null) {
261276
closeConnection(this.target);
262277
}
@@ -267,19 +282,26 @@ public void initConnection() throws SQLException {
267282
}
268283
this.connection = (isSuppressClose() ? getCloseSuppressingConnectionProxy(this.target) : this.target);
269284
}
285+
finally {
286+
this.connectionLock.unlock();
287+
}
270288
}
271289

272290
/**
273291
* Reset the underlying shared Connection, to be reinitialized on next access.
274292
*/
275293
public void resetConnection() {
276-
synchronized (this.connectionMonitor) {
294+
this.connectionLock.lock();
295+
try {
277296
if (this.target != null) {
278297
closeConnection(this.target);
279298
}
280299
this.target = null;
281300
this.connection = null;
282301
}
302+
finally {
303+
this.connectionLock.unlock();
304+
}
283305
}
284306

285307
/**

Diff for: ‎spring-jms/src/main/java/org/springframework/jms/connection/SingleConnectionFactory.java

+65-15
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2023 the original author or authors.
2+
* Copyright 2002-2024 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -24,6 +24,8 @@
2424
import java.util.LinkedHashSet;
2525
import java.util.List;
2626
import java.util.Set;
27+
import java.util.concurrent.locks.Lock;
28+
import java.util.concurrent.locks.ReentrantLock;
2729

2830
import jakarta.jms.Connection;
2931
import jakarta.jms.ConnectionFactory;
@@ -116,8 +118,8 @@ public class SingleConnectionFactory implements ConnectionFactory, QueueConnecti
116118
/** Whether the shared Connection has been started. */
117119
private int startedCount = 0;
118120

119-
/** Synchronization monitor for the shared Connection. */
120-
private final Object connectionMonitor = new Object();
121+
/** Lifecycle lock for the shared Connection. */
122+
private final Lock connectionLock = new ReentrantLock();
121123

122124

123125
/**
@@ -252,10 +254,14 @@ public Connection createConnection(String username, String password) throws JMSE
252254
@Override
253255
public QueueConnection createQueueConnection() throws JMSException {
254256
Connection con;
255-
synchronized (this.connectionMonitor) {
257+
this.connectionLock.lock();
258+
try {
256259
this.pubSubMode = Boolean.FALSE;
257260
con = createConnection();
258261
}
262+
finally {
263+
this.connectionLock.unlock();
264+
}
259265
if (!(con instanceof QueueConnection queueConnection)) {
260266
throw new jakarta.jms.IllegalStateException(
261267
"This SingleConnectionFactory does not hold a QueueConnection but rather: " + con);
@@ -272,10 +278,14 @@ public QueueConnection createQueueConnection(String username, String password) t
272278
@Override
273279
public TopicConnection createTopicConnection() throws JMSException {
274280
Connection con;
275-
synchronized (this.connectionMonitor) {
281+
this.connectionLock.lock();
282+
try {
276283
this.pubSubMode = Boolean.TRUE;
277284
con = createConnection();
278285
}
286+
finally {
287+
this.connectionLock.unlock();
288+
}
279289
if (!(con instanceof TopicConnection topicConnection)) {
280290
throw new jakarta.jms.IllegalStateException(
281291
"This SingleConnectionFactory does not hold a TopicConnection but rather: " + con);
@@ -323,12 +333,16 @@ private ConnectionFactory obtainTargetConnectionFactory() {
323333
* @see #initConnection()
324334
*/
325335
protected Connection getConnection() throws JMSException {
326-
synchronized (this.connectionMonitor) {
336+
this.connectionLock.lock();
337+
try {
327338
if (this.connection == null) {
328339
initConnection();
329340
}
330341
return this.connection;
331342
}
343+
finally {
344+
this.connectionLock.unlock();
345+
}
332346
}
333347

334348
/**
@@ -386,9 +400,13 @@ public void stop() {
386400
*/
387401
@Override
388402
public boolean isRunning() {
389-
synchronized (this.connectionMonitor) {
403+
this.connectionLock.lock();
404+
try {
390405
return (this.connection != null);
391406
}
407+
finally {
408+
this.connectionLock.unlock();
409+
}
392410
}
393411

394412

@@ -404,7 +422,8 @@ public void initConnection() throws JMSException {
404422
throw new IllegalStateException(
405423
"'targetConnectionFactory' is required for lazily initializing a Connection");
406424
}
407-
synchronized (this.connectionMonitor) {
425+
this.connectionLock.lock();
426+
try {
408427
if (this.connection != null) {
409428
closeConnection(this.connection);
410429
}
@@ -433,6 +452,9 @@ public void initConnection() throws JMSException {
433452
logger.debug("Established shared JMS Connection: " + this.connection);
434453
}
435454
}
455+
finally {
456+
this.connectionLock.unlock();
457+
}
436458
}
437459

438460
/**
@@ -531,12 +553,16 @@ else if (Boolean.TRUE.equals(this.pubSubMode) && con instanceof TopicConnection
531553
* @see #closeConnection
532554
*/
533555
public void resetConnection() {
534-
synchronized (this.connectionMonitor) {
556+
this.connectionLock.lock();
557+
try {
535558
if (this.connection != null) {
536559
closeConnection(this.connection);
537560
}
538561
this.connection = null;
539562
}
563+
finally {
564+
this.connectionLock.unlock();
565+
}
540566
}
541567

542568
/**
@@ -634,7 +660,8 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
634660
}
635661
case "setExceptionListener" -> {
636662
// Handle setExceptionListener method: add to the chain.
637-
synchronized (connectionMonitor) {
663+
connectionLock.lock();
664+
try {
638665
if (aggregatedExceptionListener != null) {
639666
ExceptionListener listener = (ExceptionListener) args[0];
640667
if (listener != this.localExceptionListener) {
@@ -656,16 +683,23 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
656683
"which will allow for registering further ExceptionListeners to the recovery chain.");
657684
}
658685
}
686+
finally {
687+
connectionLock.unlock();
688+
}
659689
}
660690
case "getExceptionListener" -> {
661-
synchronized (connectionMonitor) {
691+
connectionLock.lock();
692+
try {
662693
if (this.localExceptionListener != null) {
663694
return this.localExceptionListener;
664695
}
665696
else {
666697
return getExceptionListener();
667698
}
668699
}
700+
finally {
701+
connectionLock.unlock();
702+
}
669703
}
670704
case "start" -> {
671705
localStart();
@@ -677,14 +711,18 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
677711
}
678712
case "close" -> {
679713
localStop();
680-
synchronized (connectionMonitor) {
714+
connectionLock.lock();
715+
try {
681716
if (this.localExceptionListener != null) {
682717
if (aggregatedExceptionListener != null) {
683718
aggregatedExceptionListener.delegates.remove(this.localExceptionListener);
684719
}
685720
this.localExceptionListener = null;
686721
}
687722
}
723+
finally {
724+
connectionLock.unlock();
725+
}
688726
return null;
689727
}
690728
case "createSession", "createQueueSession", "createTopicSession" -> {
@@ -727,7 +765,8 @@ else if (args.length == 2) {
727765
}
728766

729767
private void localStart() throws JMSException {
730-
synchronized (connectionMonitor) {
768+
connectionLock.lock();
769+
try {
731770
if (!this.locallyStarted) {
732771
this.locallyStarted = true;
733772
if (startedCount == 0 && connection != null) {
@@ -736,10 +775,14 @@ private void localStart() throws JMSException {
736775
startedCount++;
737776
}
738777
}
778+
finally {
779+
connectionLock.unlock();
780+
}
739781
}
740782

741783
private void localStop() throws JMSException {
742-
synchronized (connectionMonitor) {
784+
connectionLock.lock();
785+
try {
743786
if (this.locallyStarted) {
744787
this.locallyStarted = false;
745788
if (startedCount == 1 && connection != null) {
@@ -750,6 +793,9 @@ private void localStop() throws JMSException {
750793
}
751794
}
752795
}
796+
finally {
797+
connectionLock.unlock();
798+
}
753799
}
754800

755801
private SingleConnectionFactory factory() {
@@ -771,9 +817,13 @@ public void onException(JMSException ex) {
771817
// Iterate over temporary copy in order to avoid ConcurrentModificationException,
772818
// since listener invocations may in turn trigger registration of listeners...
773819
Set<ExceptionListener> copy;
774-
synchronized (connectionMonitor) {
820+
connectionLock.lock();
821+
try {
775822
copy = new LinkedHashSet<>(this.delegates);
776823
}
824+
finally {
825+
connectionLock.unlock();
826+
}
777827
for (ExceptionListener listener : copy) {
778828
listener.onException(ex);
779829
}

0 commit comments

Comments
 (0)