diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index a972de65e..3de6630e1 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -7,6 +7,7 @@ ### Updated ### Fixed +- Fixed `StackOverflowError` / hang when closing a `ResultSet` or `Statement` with `closeOnCompletion()` enabled. - Fixed `setCatalog()` and `setSchema()` producing invalid SQL (e.g. `SET CATALOG ``name``) when the catalog or schema name was passed already wrapped in backticks. Backticks are now stripped before wrapping, and `getCatalog()`/`getSchema()` return the bare identifier name. - Fixed metadata SQL generation for catalog, schema, and table identifiers containing backticks. - Fixed SEA result truncation when direct results are disabled. Large, highly-compressible results that span multiple chunks were delivered inline via the old hybrid path and truncated to the first chunk. The SQL Execution path now uses an async (`0s`) wait timeout when direct results are disabled, so results are returned via external links and fetched in full. diff --git a/src/main/java/com/databricks/jdbc/api/impl/DatabricksResultSet.java b/src/main/java/com/databricks/jdbc/api/impl/DatabricksResultSet.java index 540699407..32bc29ab4 100644 --- a/src/main/java/com/databricks/jdbc/api/impl/DatabricksResultSet.java +++ b/src/main/java/com/databricks/jdbc/api/impl/DatabricksResultSet.java @@ -360,10 +360,19 @@ public boolean next() throws SQLException { @Override public void close() throws DatabricksSQLException { + // Idempotency guard. Besides making close() safe to call more than once, this also breaks the + // mutual recursion that occurs when closeOnCompletion() is enabled (ES-1978361): closing the + // ResultSet notifies the parent Statement (handleResultSetClose), which closes the Statement, + // which closes the ResultSet again. Returning early on the re-entrant call stops the loop. + if (isClosed) { + return; + } + // Mark closed before notifying the parent Statement, so any re-entrant close() triggered via + // handleResultSetClose() short-circuits at the guard above instead of recursing. + isClosed = true; stopHeartbeat(); // Proactively close server operation when ResultSet is closed explicitly. closeServerOperation(); - isClosed = true; if (executionResult != null) { executionResult.close(); } diff --git a/src/test/java/com/databricks/jdbc/api/impl/DatabricksStatementTest.java b/src/test/java/com/databricks/jdbc/api/impl/DatabricksStatementTest.java index 73ed7f33c..520de0686 100644 --- a/src/test/java/com/databricks/jdbc/api/impl/DatabricksStatementTest.java +++ b/src/test/java/com/databricks/jdbc/api/impl/DatabricksStatementTest.java @@ -2188,4 +2188,74 @@ public void testStatementClose_noDoubleRpc_whenResultSetNotClosed() throws Excep // Should fire exactly one closeStatement RPC, not two verify(client, times(1)).closeStatement(STATEMENT_ID); } + + /** + * Regression test for ES-1978361. With closeOnCompletion() enabled, closing the ResultSet must + * not recurse infinitely between ResultSet.close() and Statement.close() (previously a + * StackOverflowError). Exercises the ResultSet.close() entry point with a real ResultSet wired to + * a real Statement. + */ + @Test + public void testCloseOnCompletion_resultSetCloseDoesNotRecurse() throws Exception { + IDatabricksConnectionContext connectionContext = + DatabricksConnectionContext.parse(JDBC_URL, new Properties()); + DatabricksConnection connection = new DatabricksConnection(connectionContext, client); + DatabricksStatement statement = new DatabricksStatement(connection); + + IExecutionResult execResult = mock(IExecutionResult.class); + DatabricksResultSetMetaData rsMeta = mock(DatabricksResultSetMetaData.class); + DatabricksResultSet resultSet = + new DatabricksResultSet( + new StatementStatus().setState(StatementState.SUCCEEDED), + STATEMENT_ID, + StatementType.QUERY, + statement, + execResult, + rsMeta, + false); + statement.setStatementId(STATEMENT_ID); + statement.resultSet = resultSet; + statement.closeOnCompletion(); + assertTrue(statement.isCloseOnCompletion()); + + // Closing the ResultSet auto-closes the Statement (closeOnCompletion). Must not StackOverflow. + assertDoesNotThrow(() -> resultSet.close()); + + assertTrue(resultSet.isClosed()); + assertTrue(statement.isClosed()); + // Server operation must be closed at most once despite the close path being entered twice. + verify(client, atMost(1)).closeStatement(STATEMENT_ID); + } + + /** + * Regression test for ES-1978361, statement-close entry point. Closing the Statement (which + * closes its ResultSet, which calls back via closeOnCompletion) must not recurse infinitely. + */ + @Test + public void testCloseOnCompletion_statementCloseDoesNotRecurse() throws Exception { + IDatabricksConnectionContext connectionContext = + DatabricksConnectionContext.parse(JDBC_URL, new Properties()); + DatabricksConnection connection = new DatabricksConnection(connectionContext, client); + DatabricksStatement statement = new DatabricksStatement(connection); + + IExecutionResult execResult = mock(IExecutionResult.class); + DatabricksResultSetMetaData rsMeta = mock(DatabricksResultSetMetaData.class); + DatabricksResultSet resultSet = + new DatabricksResultSet( + new StatementStatus().setState(StatementState.SUCCEEDED), + STATEMENT_ID, + StatementType.QUERY, + statement, + execResult, + rsMeta, + false); + statement.setStatementId(STATEMENT_ID); + statement.resultSet = resultSet; + statement.closeOnCompletion(); + + assertDoesNotThrow(() -> statement.close()); + + assertTrue(statement.isClosed()); + assertTrue(resultSet.isClosed()); + } }