Skip to content

Commit f3b1a9e

Browse files
committed
Do not allow to cancel other running stmts (1.3)
This is a backport of the PR duckdb#245 to `v1.3-ossivalis` stable branch. `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 ee1bc2c commit f3b1a9e

File tree

3 files changed

+74
-14
lines changed

3 files changed

+74
-14
lines changed

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

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

3838
ByteBuffer connRef;
39-
final Lock connRefLock = new ReentrantLock();
39+
final ReentrantLock connRefLock = new ReentrantLock();
4040
final LinkedHashSet<DuckDBPreparedStatement> preparedStatements = new LinkedHashSet<>();
4141
volatile boolean closing = false;
4242

@@ -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/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
}

0 commit comments

Comments
 (0)