[GitHub] [shardingsphere] TeslaCN commented on a diff in pull request #18644: fix -hikari connection leak

[GitBox <gi...@apache.org>]

TeslaCN commented on code in PR #18644:
URL: https://github.com/apache/shardingsphere/pull/18644#discussion_r908139510


##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/connection/JDBCBackendConnection.java:
##########
@@ -64,10 +65,13 @@ public final class JDBCBackendConnection implements BackendConnection<Void>, Exe
     
     private final ResourceLock resourceLock = new ResourceLock();
     
+    private final AtomicBoolean closed;
+    
     private volatile int connectionReferenceCount;
     
     public JDBCBackendConnection(final ConnectionSession connectionSession) {
         this.connectionSession = connectionSession;
+        this.closed = new AtomicBoolean(false);

Review Comment:
   Please remove the redundant `this`.



##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/connection/JDBCBackendConnection.java:
##########
@@ -203,7 +207,11 @@ public Void closeExecutionResources() throws BackendConnectionException {
             if (!connectionSession.getTransactionStatus().isInConnectionHeldTransaction()) {
                 result.addAll(closeDatabaseCommunicationEngines(true));
                 result.addAll(closeConnections(false));
+            } else if (this.closed.get()) {

Review Comment:
   Please remove the redundant `this`.



##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/connection/JDBCBackendConnection.java:
##########
@@ -214,6 +222,7 @@ public Void closeExecutionResources() throws BackendConnectionException {
     @Override
     public Void closeAllResources() {
         synchronized (this) {
+            this.closed.set(true);

Review Comment:
   Please remove the redundant `this`.



##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/communication/vertx/VertxBackendConnection.java:
##########
@@ -131,12 +135,15 @@ public Future<Void> handleAutoCommit() {
     public Future<Void> closeExecutionResources() {
         if (!connectionSession.getTransactionStatus().isInTransaction()) {
             return closeAllConnections(false);
+        } else if (this.closed.get()) {
+            return closeAllConnections(true);
         }
         return Future.succeededFuture();
     }
     
     @Override
     public Future<Void> closeAllResources() {
+        this.closed.set(true);

Review Comment:
   Please remove the redundant `this`.



##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/connection/JDBCBackendConnection.java:
##########
@@ -203,7 +207,11 @@ public Void closeExecutionResources() throws BackendConnectionException {
             if (!connectionSession.getTransactionStatus().isInConnectionHeldTransaction()) {
                 result.addAll(closeDatabaseCommunicationEngines(true));
                 result.addAll(closeConnections(false));
+            } else if (this.closed.get()) {
+                result.addAll(closeDatabaseCommunicationEngines(true));
+                result.addAll(closeConnections(true));
             }
+            

Review Comment:
   Please remove the empty line.



##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/communication/vertx/VertxBackendConnection.java:
##########
@@ -131,12 +135,15 @@ public Future<Void> handleAutoCommit() {
     public Future<Void> closeExecutionResources() {
         if (!connectionSession.getTransactionStatus().isInTransaction()) {
             return closeAllConnections(false);
+        } else if (this.closed.get()) {

Review Comment:
   Please remove the redundant `this`.



##########
shardingsphere-proxy/shardingsphere-proxy-backend/src/main/java/org/apache/shardingsphere/proxy/backend/communication/vertx/VertxBackendConnection.java:
##########
@@ -52,10 +53,13 @@ public final class VertxBackendConnection implements BackendConnection<Future<Vo
     
     private final Multimap<String, Future<SqlConnection>> cachedConnections = LinkedHashMultimap.create();
     
+    private final AtomicBoolean closed;
+    
     public VertxBackendConnection(final ConnectionSession connectionSession) {
         if (TransactionType.LOCAL != connectionSession.getTransactionStatus().getTransactionType()) {
             throw new UnsupportedOperationException("Vert.x backend supports LOCAL transaction only for now.");
         }
+        this.closed = new AtomicBoolean(false);

Review Comment:
   Please remove the redundant `this`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org