Skip to content

Commit 44264d0

Browse files
committed
Do not allow to cancel other running statements
`Statement#cancel()` call works on connection level. So currently it is well possible to run query in one statement, and then interrupt this query calling cancel on another statement. This change adds a check that cancellation can only be performed if the query on current statement is still running. Otherwise `stmt.cancel()` call is a no-op. Testing: new test added that checks that other statement cannot be cancelled.
1 parent 5243e14 commit 44264d0

File tree

5 files changed

+76
-71
lines changed

5 files changed

+76
-71
lines changed

src/main/java/org/duckdb/DuckDBConnection.java

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ public final class DuckDBConnection implements java.sql.Connection {
3737
public static final String DEFAULT_SCHEMA = "main";
3838

3939
ByteBuffer connRef;
40-
final Lock connRefLock = new ReentrantLock();
40+
final ReentrantLock connRefLock = new ReentrantLock();
4141
final LinkedHashSet<DuckDBPreparedStatement> preparedStatements = new LinkedHashSet<>();
4242
volatile boolean closing;
4343

@@ -488,14 +488,11 @@ void checkOpen() throws SQLException {
488488
* This function calls the underlying C++ interrupt function which aborts the query running on this connection.
489489
*/
490490
void interrupt() throws SQLException {
491-
checkOpen();
492-
connRefLock.lock();
493-
try {
494-
checkOpen();
495-
DuckDBNative.duckdb_jdbc_interrupt(connRef);
496-
} finally {
497-
connRefLock.unlock();
491+
if (!connRefLock.isHeldByCurrentThread()) {
492+
throw new SQLException("Connection lock state error");
498493
}
494+
checkOpen();
495+
DuckDBNative.duckdb_jdbc_interrupt(connRef);
499496
}
500497

501498
QueryProgress queryProgress() throws SQLException {

src/main/java/org/duckdb/DuckDBDriver.java

Lines changed: 2 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ public class DuckDBDriver implements java.sql.Driver {
1616
public static final String JDBC_STREAM_RESULTS = "jdbc_stream_results";
1717
public static final String JDBC_AUTO_COMMIT = "jdbc_auto_commit";
1818
public static final String JDBC_PIN_DB = "jdbc_pin_db";
19-
public static final String JDBC_IGNORE_UNSUPPORTED_OPTIONS = "jdbc_ignore_unsupported_options";
2019

2120
static final String DUCKDB_URL_PREFIX = "jdbc:duckdb:";
2221
static final String MEMORY_DB = ":memory:";
@@ -34,9 +33,6 @@ public class DuckDBDriver implements java.sql.Driver {
3433
private static boolean pinnedDbRefsShutdownHookRegistered = false;
3534
private static boolean pinnedDbRefsShutdownHookRun = false;
3635

37-
private static final Set<String> supportedOptions = new LinkedHashSet<>();
38-
private static final ReentrantLock supportedOptionsLock = new ReentrantLock();
39-
4036
static {
4137
try {
4238
DriverManager.registerDriver(new DuckDBDriver());
@@ -56,20 +52,13 @@ public Connection connect(String url, Properties info) throws SQLException {
5652
props = (Properties) info.clone();
5753
}
5854

59-
// URL options
6055
ParsedProps pp = parsePropsFromUrl(url);
6156
for (Map.Entry<String, String> en : pp.props.entrySet()) {
6257
props.put(en.getKey(), en.getValue());
6358
}
6459

65-
// Ignore unsupported
66-
removeUnsupportedOptions(props);
67-
68-
// Read-only option
6960
String readOnlyStr = removeOption(props, DUCKDB_READONLY_PROPERTY);
7061
boolean readOnly = isStringTruish(readOnlyStr, false);
71-
72-
// Client name option
7362
props.put("duckdb_api", "jdbc");
7463

7564
// Apache Spark passes this option when SELECT on a JDBC DataSource
@@ -78,7 +67,6 @@ public Connection connect(String url, Properties info) throws SQLException {
7867
// to be established.
7968
props.remove("path");
8069

81-
// DuckLake options
8270
String ducklake = removeOption(props, DUCKLAKE_OPTION);
8371
String ducklakeAlias = removeOption(props, DUCKLAKE_ALIAS_OPTION, DUCKLAKE_OPTION);
8472
final String shortUrl;
@@ -95,13 +83,13 @@ public Connection connect(String url, Properties info) throws SQLException {
9583
shortUrl = pp.shortUrl;
9684
}
9785

98-
// Pin DB option
9986
String pinDbOptStr = removeOption(props, JDBC_PIN_DB);
10087
boolean pinDBOpt = isStringTruish(pinDbOptStr, false);
10188

102-
// Create connection
10389
DuckDBConnection conn = DuckDBConnection.newConnection(shortUrl, readOnly, props);
90+
10491
pinDB(pinDBOpt, shortUrl, conn);
92+
10593
initDucklake(conn, shortUrl, ducklake, ducklakeAlias);
10694

10795
return conn;
@@ -128,8 +116,6 @@ public DriverPropertyInfo[] getPropertyInfo(String url, Properties info) throws
128116
list.add(createDriverPropInfo(JDBC_AUTO_COMMIT, "", "Set default auto-commit mode"));
129117
list.add(createDriverPropInfo(JDBC_PIN_DB, "",
130118
"Do not close the DB instance after all connections to it are closed"));
131-
list.add(createDriverPropInfo(JDBC_IGNORE_UNSUPPORTED_OPTIONS, "",
132-
"Silently discard unsupported connection options"));
133119
list.sort((o1, o2) -> o1.name.compareToIgnoreCase(o2.name));
134120
return list.toArray(new DriverPropertyInfo[0]);
135121
}
@@ -265,38 +251,6 @@ private static DriverPropertyInfo createDriverPropInfo(String name, String value
265251
return dpi;
266252
}
267253

268-
private static void removeUnsupportedOptions(Properties props) throws SQLException {
269-
String ignoreStr = removeOption(props, JDBC_IGNORE_UNSUPPORTED_OPTIONS);
270-
boolean ignore = isStringTruish(ignoreStr, false);
271-
if (!ignore) {
272-
return;
273-
}
274-
supportedOptionsLock.lock();
275-
try {
276-
if (supportedOptions.isEmpty()) {
277-
Driver driver = DriverManager.getDriver(DUCKDB_URL_PREFIX);
278-
Properties dpiProps = new Properties();
279-
dpiProps.put("threads", 1);
280-
DriverPropertyInfo[] dpis = driver.getPropertyInfo(DUCKDB_URL_PREFIX, dpiProps);
281-
for (DriverPropertyInfo dpi : dpis) {
282-
supportedOptions.add(dpi.name);
283-
}
284-
}
285-
List<String> unsupportedNames = new ArrayList<>();
286-
for (Object nameObj : props.keySet()) {
287-
String name = String.valueOf(nameObj);
288-
if (!supportedOptions.contains(name)) {
289-
unsupportedNames.add(name);
290-
}
291-
}
292-
for (String name : unsupportedNames) {
293-
props.remove(name);
294-
}
295-
} finally {
296-
supportedOptionsLock.unlock();
297-
}
298-
}
299-
300254
private static class ParsedProps {
301255
final String shortUrl;
302256
final LinkedHashMap<String, String> props;

src/main/java/org/duckdb/DuckDBPreparedStatement.java

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ public class DuckDBPreparedStatement implements PreparedStatement {
4444
private DuckDBConnection conn;
4545

4646
private ByteBuffer stmtRef = null;
47-
final Lock stmtRefLock = new ReentrantLock();
47+
final ReentrantLock stmtRefLock = new ReentrantLock();
4848
volatile boolean closeOnCompletion = false;
4949

5050
private DuckDBResultSet selectResult = null;
@@ -159,6 +159,11 @@ private boolean execute(boolean startTransaction) throws SQLException {
159159
checkOpen();
160160
checkPrepared();
161161

162+
// Wait with dispatching a new query if connection is locked by cancel() call
163+
Lock connLock = getConnRefLock();
164+
connLock.lock();
165+
connLock.unlock();
166+
162167
ByteBuffer resultRef = null;
163168

164169
stmtRefLock.lock();
@@ -442,12 +447,27 @@ public void setQueryTimeout(int seconds) throws SQLException {
442447
@Override
443448
public void cancel() throws SQLException {
444449
checkOpen();
450+
// Only proceed to interrupt call after ensuring that the query on
451+
// this statement is still running.
452+
if (!stmtRefLock.isLocked()) {
453+
return;
454+
}
455+
// Cancel is intended to be called concurrently with execute,
456+
// thus we cannot take the statement lock that is held while
457+
// query is running. NPE may be thrown if connection is closed
458+
// concurrently.
445459
try {
446-
// Cancel is intended to be called concurrently with execute,
447-
// thus we cannot take the statement lock that is held while
448-
// query is running. NPE may be thrown if connection is closed
449-
// concurrently.
450-
conn.interrupt();
460+
// Taking connection lock will prevent new queries to be executed
461+
Lock connLock = getConnRefLock();
462+
connLock.lock();
463+
try {
464+
if (!stmtRefLock.isLocked()) {
465+
return;
466+
}
467+
conn.interrupt();
468+
} finally {
469+
connLock.unlock();
470+
}
451471
} catch (NullPointerException e) {
452472
throw new SQLException(e);
453473
}
@@ -1215,4 +1235,13 @@ private int[] intArrayFromLong(long[] arr) {
12151235
}
12161236
return res;
12171237
}
1238+
1239+
private Lock getConnRefLock() throws SQLException {
1240+
// NPE can be thrown if statement is closed concurrently.
1241+
try {
1242+
return conn.connRefLock;
1243+
} catch (NullPointerException e) {
1244+
throw new SQLException(e);
1245+
}
1246+
}
12181247
}

src/test/java/org/duckdb/TestClosure.java

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,4 +251,38 @@ public static void test_results_fetch_no_hang() throws Exception {
251251
}
252252
}
253253
}
254+
255+
public static void test_stmt_can_only_cancel_self() throws Exception {
256+
try (Connection conn = DriverManager.getConnection(JDBC_URL); Statement stmt1 = conn.createStatement();
257+
Statement stmt2 = conn.createStatement()) {
258+
stmt1.execute("DROP TABLE IF EXISTS test_fib1");
259+
stmt1.execute("CREATE TABLE test_fib1(i bigint, p double, f double)");
260+
stmt1.execute("INSERT INTO test_fib1 values(1, 0, 1)");
261+
long start = System.currentTimeMillis();
262+
Thread th = new Thread(() -> {
263+
try {
264+
Thread.sleep(200);
265+
stmt1.cancel();
266+
} catch (Exception e) {
267+
e.printStackTrace();
268+
}
269+
});
270+
th.start();
271+
try (
272+
ResultSet rs = stmt2.executeQuery(
273+
"WITH RECURSIVE cte AS ("
274+
+
275+
"SELECT * from test_fib1 UNION ALL SELECT cte.i + 1, cte.f, cte.p + cte.f from cte WHERE cte.i < 40000) "
276+
+ "SELECT avg(f) FROM cte")) {
277+
rs.next();
278+
assertTrue(rs.getDouble(1) > 0);
279+
}
280+
th.join();
281+
long elapsed = System.currentTimeMillis() - start;
282+
assertTrue(elapsed > 1000);
283+
assertFalse(conn.isClosed());
284+
assertFalse(stmt1.isClosed());
285+
assertFalse(stmt2.isClosed());
286+
}
287+
}
254288
}

src/test/java/org/duckdb/TestDuckDBJDBC.java

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
import static java.util.Collections.emptyList;
1111
import static java.util.Collections.singletonList;
1212
import static org.duckdb.DuckDBDriver.DUCKDB_USER_AGENT_PROPERTY;
13-
import static org.duckdb.DuckDBDriver.JDBC_STREAM_RESULTS;
1413
import static org.duckdb.DuckDBTimestamp.localDateTimeFromTimestamp;
1514
import static org.duckdb.test.Assertions.*;
1615
import static org.duckdb.test.Runner.runTests;
@@ -3657,14 +3656,6 @@ public static void test_driver_property_info() throws Exception {
36573656
assertTrue(dpis.length > 0);
36583657
}
36593658

3660-
public static void test_ignore_unsupported_options() throws Exception {
3661-
assertThrows(() -> { DriverManager.getConnection("jdbc:duckdb:;foo=bar;"); }, SQLException.class);
3662-
Properties config = new Properties();
3663-
config.put("boo", "bar");
3664-
config.put(JDBC_STREAM_RESULTS, true);
3665-
DriverManager.getConnection("jdbc:duckdb:;foo=bar;jdbc_ignore_unsupported_options=yes;", config).close();
3666-
}
3667-
36683659
public static void main(String[] args) throws Exception {
36693660
String arg1 = args.length > 0 ? args[0] : "";
36703661
final int statusCode;

0 commit comments

Comments
 (0)