Skip to content

Commit 7e61d46

Browse files
authored
GH-932: [JDBC] Fix memory leak on Connection#close due to unclosed Statement(s) (#933)
## What's Changed Closing a Connection when there was one or more ResultSet that matched the following 2 conditions 1. hadn't been fully consumed 2. was obtained via a Statement instance of this Connection instance would generate exceptions due to memory leaks. Now, closing a Connection will first close all the Statement instances obtained via that Connection, which has a side effect of closing all the ResultSet, and then proceed with the old closing logic. This side effect is guaranteed by the JDBC Spec 4.3, chapter 13.1.4 The old closing logic was also slightly refactored to: 1. remove duplicate calls to ArrowFlightSqlClientHandler.close() 5. make sure that any exception generated during Connection.close() would be wrapped in a SQLException. Closes #932.
1 parent a1d8317 commit 7e61d46

File tree

2 files changed

+68
-9
lines changed

2 files changed

+68
-9
lines changed

flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/ArrowFlightConnection.java

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import io.netty.util.concurrent.DefaultThreadFactory;
2222
import java.sql.SQLException;
23+
import java.util.ArrayList;
2324
import java.util.Properties;
2425
import java.util.concurrent.ExecutorService;
2526
import java.util.concurrent.Executors;
@@ -180,19 +181,39 @@ public Properties getClientInfo() {
180181

181182
@Override
182183
public void close() throws SQLException {
183-
clientHandler.close();
184-
if (executorService != null) {
185-
executorService.shutdown();
184+
Exception topLevelException = null;
185+
try {
186+
if (executorService != null) {
187+
executorService.shutdown();
188+
}
189+
} catch (final Exception e) {
190+
topLevelException = e;
191+
}
192+
ArrayList<AutoCloseable> closeables = new ArrayList<>(statementMap.values());
193+
closeables.add(clientHandler);
194+
closeables.addAll(allocator.getChildAllocators());
195+
closeables.add(allocator);
196+
try {
197+
AutoCloseables.close(closeables);
198+
} catch (final Exception e) {
199+
if (topLevelException == null) {
200+
topLevelException = e;
201+
} else {
202+
topLevelException.addSuppressed(e);
203+
}
186204
}
187-
188205
try {
189-
AutoCloseables.close(clientHandler);
190-
allocator.getChildAllocators().forEach(AutoCloseables::closeNoChecked);
191-
AutoCloseables.close(allocator);
192-
193206
super.close();
194207
} catch (final Exception e) {
195-
throw AvaticaConnection.HELPER.createException(e.getMessage(), e);
208+
if (topLevelException == null) {
209+
topLevelException = e;
210+
} else {
211+
topLevelException.addSuppressed(e);
212+
}
213+
}
214+
if (topLevelException != null) {
215+
throw AvaticaConnection.HELPER.createException(
216+
topLevelException.getMessage(), topLevelException);
196217
}
197218
}
198219

flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ConnectionTest.java

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
package org.apache.arrow.driver.jdbc;
1818

1919
import static org.junit.jupiter.api.Assertions.assertEquals;
20+
import static org.junit.jupiter.api.Assertions.assertFalse;
2021
import static org.junit.jupiter.api.Assertions.assertNotNull;
2122
import static org.junit.jupiter.api.Assertions.assertThrows;
2223
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -27,6 +28,7 @@
2728
import java.sql.Driver;
2829
import java.sql.DriverManager;
2930
import java.sql.SQLException;
31+
import java.sql.Statement;
3032
import java.util.Map;
3133
import java.util.Properties;
3234
import org.apache.arrow.driver.jdbc.authentication.UserPasswordAuthentication;
@@ -660,4 +662,40 @@ public String visit(String value) {
660662
assertEquals(catalog, actualCatalog);
661663
}
662664
}
665+
666+
@Test
667+
public void testStatementsClosedOnConnectionClose() throws Exception {
668+
// create a connection
669+
final Properties properties = new Properties();
670+
properties.put(ArrowFlightConnectionProperty.HOST.camelName(), "localhost");
671+
properties.put(
672+
ArrowFlightConnectionProperty.PORT.camelName(), FLIGHT_SERVER_TEST_EXTENSION.getPort());
673+
properties.put(ArrowFlightConnectionProperty.USER.camelName(), userTest);
674+
properties.put(ArrowFlightConnectionProperty.PASSWORD.camelName(), passTest);
675+
properties.put("useEncryption", false);
676+
677+
Connection connection =
678+
DriverManager.getConnection(
679+
"jdbc:arrow-flight-sql://"
680+
+ FLIGHT_SERVER_TEST_EXTENSION.getHost()
681+
+ ":"
682+
+ FLIGHT_SERVER_TEST_EXTENSION.getPort(),
683+
properties);
684+
685+
// create some statements
686+
int numStatements = 3;
687+
Statement[] statements = new Statement[numStatements];
688+
for (int i = 0; i < numStatements; i++) {
689+
statements[i] = connection.createStatement();
690+
assertFalse(statements[i].isClosed());
691+
}
692+
693+
// close the connection
694+
connection.close();
695+
696+
// assert the statements are closed
697+
for (int i = 0; i < numStatements; i++) {
698+
assertTrue(statements[i].isClosed());
699+
}
700+
}
663701
}

0 commit comments

Comments
 (0)