diff --git a/client-v2/src/main/java/com/clickhouse/client/api/ServerException.java b/client-v2/src/main/java/com/clickhouse/client/api/ServerException.java index 0abfabe39..2f5acb34f 100644 --- a/client-v2/src/main/java/com/clickhouse/client/api/ServerException.java +++ b/client-v2/src/main/java/com/clickhouse/client/api/ServerException.java @@ -10,6 +10,8 @@ public class ServerException extends ClickHouseException { public static final int EXECUTION_TIMEOUT = 159; + public static final int NOTHING_TO_KILL = 785; + private final int code; private final int transportProtocolCode; diff --git a/client-v2/src/main/java/com/clickhouse/client/api/internal/ClientUtils.java b/client-v2/src/main/java/com/clickhouse/client/api/internal/ClientUtils.java index 9814c27b1..40314c521 100644 --- a/client-v2/src/main/java/com/clickhouse/client/api/internal/ClientUtils.java +++ b/client-v2/src/main/java/com/clickhouse/client/api/internal/ClientUtils.java @@ -11,6 +11,8 @@ */ public final class ClientUtils { + private static final int DEFAULT_CAUSE_SEARCH_DEPTH = 3; + private ClientUtils() {} public static boolean isNotBlank(String str) { @@ -50,4 +52,46 @@ public static void putIfAbsentIgnoreCase(Map map, String key, V v } map.put(key, value); } + + /** + * Finds the nearest exception of the given type in the cause chain. Checks {@code throwable} first and then + * follows up to 3 causes. + * + * @param throwable - exception to start from, can be {@code null} + * @param type - type of the exception to find, not {@code null} + * @param - type of the exception to find + * @return {@code throwable} or its cause that is an instance of {@code type}, or {@code null} if not found + */ + public static T findCause(Throwable throwable, Class type) { + return findCause(throwable, type, DEFAULT_CAUSE_SEARCH_DEPTH); + } + + /** + * Finds the nearest exception of the given type in the cause chain. Checks {@code throwable} first and then + * follows up to {@code maxDepth} causes. + * + * @param throwable - exception to start from, can be {@code null} + * @param type - type of the exception to find, not {@code null} + * @param maxDepth - maximum number of causes to follow; {@code 0} checks only {@code throwable} + * @param - type of the exception to find + * @return {@code throwable} or its cause that is an instance of {@code type}, or {@code null} if not found + * @throws IllegalArgumentException if {@code maxDepth} is negative + */ + public static T findCause(Throwable throwable, Class type, int maxDepth) { + Objects.requireNonNull(type, "type"); + if (maxDepth < 0) { + throw new IllegalArgumentException("maxDepth must not be negative: " + maxDepth); + } + Throwable current = throwable; + for (int depth = 0; current != null; depth++) { + if (type.isInstance(current)) { + return type.cast(current); + } + if (depth == maxDepth) { + break; + } + current = current.getCause(); + } + return null; + } } diff --git a/client-v2/src/test/java/com/clickhouse/client/api/internal/ClientUtilsTest.java b/client-v2/src/test/java/com/clickhouse/client/api/internal/ClientUtilsTest.java index 7be73b265..d26c75e82 100644 --- a/client-v2/src/test/java/com/clickhouse/client/api/internal/ClientUtilsTest.java +++ b/client-v2/src/test/java/com/clickhouse/client/api/internal/ClientUtilsTest.java @@ -1,5 +1,6 @@ package com.clickhouse.client.api.internal; +import com.clickhouse.client.api.ServerException; import org.mockito.Mockito; import org.slf4j.Logger; import org.testng.Assert; @@ -80,4 +81,58 @@ public void testPutIfAbsentIgnoreCaseNullKey() { Assert.expectThrows(NullPointerException.class, () -> ClientUtils.putIfAbsentIgnoreCase(map, null, "added")); Assert.assertTrue(map.isEmpty()); } + + @Test(groups = {"unit"}, dataProvider = "findCause") + public void testFindCause(int wrappers, Integer maxDepth, boolean found) { + ServerException target = new ServerException(ServerException.NOTHING_TO_KILL, "No query to kill", 500, "q1"); + Throwable throwable = target; + for (int i = 0; i < wrappers; i++) { + throwable = new RuntimeException("wrapper " + i, throwable); + } + + ServerException actual = maxDepth == null + ? ClientUtils.findCause(throwable, ServerException.class) + : ClientUtils.findCause(throwable, ServerException.class, maxDepth); + + Assert.assertSame(actual, found ? target : null); + } + + @DataProvider(name = "findCause") + public static Object[][] findCauseData() { + return new Object[][]{ + {0, null, true}, + {3, null, true}, + {4, null, false}, + {0, 0, true}, + {1, 0, false}, + {5, 5, true}, + }; + } + + @Test(groups = {"unit"}) + public void testFindCauseReturnsNearestMatch() { + IllegalArgumentException outer = new IllegalArgumentException("outer", new IllegalStateException("inner")); + + Assert.assertSame(ClientUtils.findCause(new Exception(outer), RuntimeException.class), outer); + } + + @Test(groups = {"unit"}, timeOut = 10000) + public void testFindCauseWithoutMatch() { + Exception first = new Exception("first"); + Exception second = new Exception("second", first); + first.initCause(second); + + Assert.assertNull(ClientUtils.findCause(null, ServerException.class)); + Assert.assertNull(ClientUtils.findCause(new RuntimeException(new IOException()), ServerException.class)); + Assert.assertNull(ClientUtils.findCause(first, ServerException.class, 10)); + } + + @Test(groups = {"unit"}) + public void testFindCauseInvalidArguments() { + Throwable throwable = new RuntimeException(); + + Assert.expectThrows(NullPointerException.class, () -> ClientUtils.findCause(throwable, null)); + Assert.expectThrows(IllegalArgumentException.class, + () -> ClientUtils.findCause(throwable, RuntimeException.class, -1)); + } } diff --git a/docs/features.md b/docs/features.md index ab14fd4cc..b46ae370e 100644 --- a/docs/features.md +++ b/docs/features.md @@ -108,6 +108,7 @@ Compatibility-sensitive traits: - Prepared statements are client-side SQL rendering, not server-side prepared statements. Changes to literal encoding or placeholder parsing are externally visible behavior. - `DatabaseMetaData#getSchemas`, `#getTables` and `#getColumns` return the same result-set columns and values with and without `jdbc_metadata_use_show_statements`, with two exceptions for the `SHOW` statements: `getTables()` returns `null` in `REMARKS` and `TYPE_SCHEM`, because `SHOW TABLES` returns no table comment and no database engine. `getColumns()` with the `SHOW` statements sends one `DESCRIBE TABLE` query for each table that matches, and skips a database or table that fails with `UNKNOWN_TABLE`, `UNKNOWN_DATABASE`, `ACCESS_DENIED` or `DATALAKE_DATABASE_ERROR` (for example, a table with column-level grants only, which `system.columns` shows with the granted columns). - JDBC `cluster_name` is compatibility-sensitive for cancellation behavior: when configured, `Statement.cancel()` issues `KILL QUERY ON CLUSTER ''`, and when omitted it falls back to a local `KILL QUERY`. +- `Statement.cancel()` does nothing when the statement has no running query (the query is finished or not started yet). The `NOTHING_TO_KILL` error, which servers with `kill_throw_if_noop` enabled (the default since ClickHouse 26.11) return for such a `KILL QUERY`, is not thrown to the caller. Other `KILL QUERY` errors are thrown as `SQLException`. - String parameters are escaped with backslash-based escaping: backslashes are doubled and single quotes are backslash-escaped before values are wrapped in single quotes. - `?` placeholder detection is SQL-aware and should not treat question marks inside quoted strings, quoted identifiers, comments, casts, or similar syntax as bind parameters. - String-like ClickHouse values have stable JDBC expectations: `String`, `FixedString`, and `Enum` values are returned as strings, while `UUID` is available both as `getString()` and `getObject(..., UUID.class)`. diff --git a/history/latest/3264.md b/history/latest/3264.md new file mode 100644 index 000000000..82dde9f69 --- /dev/null +++ b/history/latest/3264.md @@ -0,0 +1,6 @@ +- **[client-v2, jdbc-v2]** Fixed `Statement.cancel()` to not throw an `SQLException` when the statement has no running + query (the query is finished or not started yet). Since ClickHouse 26.11, `kill_throw_if_noop` is enabled by default, + and the server returns `NOTHING_TO_KILL` for the `KILL QUERY` that `cancel()` sends. `cancel()` now ignores this + error. Other `KILL QUERY` errors (for example, an unknown `cluster_name`) are thrown as before. Added the + `ServerException.NOTHING_TO_KILL` error code constant to `client-v2`. + (https://github.com/ClickHouse/clickhouse-java/issues/3264) diff --git a/jdbc-v2/src/main/java/com/clickhouse/jdbc/StatementImpl.java b/jdbc-v2/src/main/java/com/clickhouse/jdbc/StatementImpl.java index c615ec4d1..4ceb9da88 100644 --- a/jdbc-v2/src/main/java/com/clickhouse/jdbc/StatementImpl.java +++ b/jdbc-v2/src/main/java/com/clickhouse/jdbc/StatementImpl.java @@ -4,6 +4,7 @@ import com.clickhouse.client.api.ServerException; import com.clickhouse.client.api.data_formats.ClickHouseFormatReader; import com.clickhouse.client.api.data_formats.JSONEachRowFormatReader; +import com.clickhouse.client.api.internal.ClientUtils; import com.clickhouse.client.api.internal.ServerSettings; import com.clickhouse.client.api.query.QueryResponse; import com.clickhouse.client.api.query.QuerySettings; @@ -522,11 +523,19 @@ public void cancel() throws SQLException { // KILL QUERY must not run inside the same session as the query being canceled otherwise it will // cause "Session is locked by a concurrent client" (SESSION_IS_LOCKED) error. QuerySettings cancelSettings = QuerySettings.merge(getLocalSettings(), new QuerySettings()).clearSession(); + final String queryId = lastQueryId; try (QueryResponse response = connection.getClient().query(String.format("KILL QUERY%sWHERE query_id = '%s'", connection.onCluster ? " ON CLUSTER " + SQLUtils.enquoteIdentifier(connection.cluster, true) + ' ' : ' ', - lastQueryId), cancelSettings).get()){ - LOG.debug("Query {} was killed by {}", lastQueryId, response.getQueryId()); + queryId), cancelSettings).get()){ + LOG.debug("Query {} was killed by {}", queryId, response.getQueryId()); } catch (Exception e) { + ServerException se = ClientUtils.findCause(e, ServerException.class); + if (se != null && se.getCode() == ServerException.NOTHING_TO_KILL) { + // The query is not running (it is finished or not started yet). A server with `kill_throw_if_noop` + // enabled (the default since 26.11) reports this as an error, but there is nothing to cancel. + LOG.debug("Query {} was not killed because it is not running: {}", queryId, se.getMessage()); + return; + } throw new SQLException(e); } } diff --git a/jdbc-v2/src/test/java/com/clickhouse/jdbc/StatementTest.java b/jdbc-v2/src/test/java/com/clickhouse/jdbc/StatementTest.java index 0657558df..fbfb81589 100644 --- a/jdbc-v2/src/test/java/com/clickhouse/jdbc/StatementTest.java +++ b/jdbc-v2/src/test/java/com/clickhouse/jdbc/StatementTest.java @@ -2,6 +2,7 @@ import com.clickhouse.client.ClickHouseServerForTest; import com.clickhouse.client.api.ClientConfigProperties; +import com.clickhouse.client.api.ServerException; import com.clickhouse.client.api.Session; import com.clickhouse.client.api.data_formats.GsonJsonParserFactory; import com.clickhouse.client.api.data_formats.JacksonJsonParserFactory; @@ -11,6 +12,10 @@ import com.clickhouse.data.ClickHouseFormat; import com.clickhouse.data.ClickHouseVersion; import com.clickhouse.jdbc.internal.SqlParserFacade; +import com.github.tomakehurst.wiremock.WireMockServer; +import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.common.ConsoleNotifier; +import com.github.tomakehurst.wiremock.core.WireMockConfiguration; import org.apache.commons.lang3.RandomStringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -781,6 +786,79 @@ public void testConcurrentCancel() throws Exception { } } + @Test(groups = {"integration"}, dataProvider = "testCancelWhenQueryIsNotRunningDP") + public void testCancelWhenQueryIsNotRunning(boolean asyncOperations) throws Exception { + Properties p = new Properties(); + p.setProperty(ClientConfigProperties.ASYNC_OPERATIONS.getKey(), String.valueOf(asyncOperations)); + if (isVersionMatch("[26.10,)")) { + p.setProperty(ClientConfigProperties.serverSetting("kill_throw_if_noop"), "1"); + } + try (Connection conn = getJdbcConnection(p); + Statement stmt = conn.createStatement()) { + stmt.cancel(); + + try (ResultSet rs = stmt.executeQuery("SELECT 1")) { + assertTrue(rs.next()); + assertEquals(rs.getInt(1), 1); + } + stmt.cancel(); + + try (ResultSet rs = stmt.executeQuery("SELECT 2")) { + assertTrue(rs.next()); + assertEquals(rs.getInt(1), 2); + } + } + + String testCluster = "test_cluster_" + RandomStringUtils.randomAlphanumeric(10); + p.setProperty(DriverProperties.CLUSTER_NAME.getKey(), testCluster); + try (Connection conn = getJdbcConnection(p); + Statement stmt = conn.createStatement()) { + SQLException e = expectThrows(SQLException.class, stmt::cancel); + assertTrue(e.getMessage().contains(testCluster), "Exception should mention the missing cluster"); + } + } + + @DataProvider + public static Object[][] testCancelWhenQueryIsNotRunningDP() { + return new Object[][] { + {false}, + {true}, + }; + } + + @Test(groups = {"integration"}, dataProvider = "testCancelWhenQueryIsNotRunningDP") + public void testCancelIgnoresNothingToKillError(boolean asyncOperations) throws Exception { + if (isCloud()) { + return; // mocked server + } + + WireMockServer mockServer = new WireMockServer(WireMockConfiguration + .options().dynamicPort().notifier(new ConsoleNotifier(false))); + mockServer.start(); + try { + // Response of a server with `kill_throw_if_noop` enabled + mockServer.addStubMapping(WireMock.post(WireMock.anyUrl()) + .withRequestBody(WireMock.matching("KILL QUERY WHERE .*")) + .willReturn(WireMock.serverError() + .withHeader("X-ClickHouse-Exception-Code", String.valueOf(ServerException.NOTHING_TO_KILL)) + .withBody("Code: 785. DB::Exception: No query to kill. (NOTHING_TO_KILL)")) + .build()); + + Properties p = new Properties(); + p.setProperty(ClientConfigProperties.COMPRESS_SERVER_RESPONSE.getKey(), "false"); + p.setProperty(ClientConfigProperties.SERVER_TIMEZONE.getKey(), "UTC"); // no server info query + p.setProperty(ClientConfigProperties.ASYNC_OPERATIONS.getKey(), String.valueOf(asyncOperations)); + try (Connection conn = new ConnectionImpl("jdbc:clickhouse://localhost:" + mockServer.port() + "/default", p); + Statement stmt = conn.createStatement()) { + stmt.cancel(); + } + mockServer.verify(1, WireMock.postRequestedFor(WireMock.anyUrl()) + .withRequestBody(WireMock.matching("KILL QUERY WHERE .*"))); + } finally { + mockServer.stop(); + } + } + /** * Waits until the given query id appears in {@code system.processes}, so we know the operation has actually * started on the server before attempting to cancel it. Uses a dedicated connection (no session) to observe.