Repository navigation
Reject statement creation on closed JDBC connections - #1752
Conversation
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
The change is mostly sound, with one medium regression: statement creation now fails fast on a closed connection, which makes setClientInfo hit a NullPointerException there. Two low findings: the feature-flag cache can still be re-created by other post-close paths, and the new test assertion pins the vendor code to 0.
There was a problem hiding this comment.
Verdict: 1 Low
The fix is correct, with one low-severity concern: setClientInfo on a closed connection is still reported as an input-validation error instead of CONNECTION_CLOSED. All statement-creation entry points now go through the closed-connection check, either directly or through the overloads that delegate to it. The parameterized regression test checks both the SQLState and vendor code, as CLAUDE.md requires. Nit: the new getStatement("statement-id") assertion in testClosedConnection only checks the exception type; it could also assert the SQLState and error code.
| String invalidConfigMessage = String.format("Configuration %s is not available", key); | ||
| String invalidValueMessage = String.format("Unsupported configuration %s=%s", key, value); | ||
| String errorMessage = e.getCause().getMessage(); | ||
| Throwable failure = e.getCause() == null ? e : e.getCause(); |
There was a problem hiding this comment.
🔵 Low — The null-safe change fixes the NPE, but on a closed connection setClientInfo still reports the failure the wrong way. setSessionConfig catches the new CONNECTION_CLOSED exception, determineClientInfoStatus turns it into REASON_UNKNOWN, and setClientInfo then throws DatabricksSQLClientInfoException with INPUT_VALIDATION_ERROR. Two consequences: telemetry logs this as a user input-validation error, and the CONNECTION_CLOSED SQLState/vendor code (1010) never reaches the caller (there's no cause chain, so the caller can't find it either).
The PR description says both overloads report SQLClientInfoException after closure. That only holds for session-conf keys like QUERY_TAGS. Keys in ALLOWED_CLIENT_INFO_PROPERTIES (e.g. ApplicationName) take the other branch, write to session.setClientInfoProperty, and succeed with no error on a closed connection.
Suggested fix: check isClosed() at the top of setClientInfo(String, String) and throw a DatabricksSQLClientInfoException with DatabricksDriverErrorCode.CONNECTION_CLOSED. That covers both branches and keeps the classification consistent with the other closed-connection checks.
There was a problem hiding this comment.
Verdict: 2 Low
Looks good overall: two low-severity concerns. The closed-connection guards on createStatement, prepareStatement and getStatement fix the feature-flag cache bug, the setClientInfo handling keeps the cause, and the fix in determineClientInfoStatus removes the null-pointer crash; the error code (1010) is tested as required. The remaining concerns are that the other closed-connection checks still report a different error code, and that the list of failed property names in the bulk setClientInfo can differ slightly from what the open-connection path would try to set.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
637171a to
4a1a52b
Compare
There was a problem hiding this comment.
Verdict: 2 Low
Looks good overall. The fix stops statement creation on closed connections from rebuilding the host-scoped feature-flag cache, and the tests cover it well. Two low-severity issues remain: some overloads build their own closed-connection exception with vendor code 0 instead of 1010, and getMetaData() may be a path the fix doesn't cover.
| if (this.isClosed()) { | ||
| throw new DatabricksSQLException( | ||
| "Connection closed!", DatabricksDriverErrorCode.CONNECTION_CLOSED); | ||
| "Connection closed!", |
There was a problem hiding this comment.
🔵 Low — This helper now reports vendor code 1010 (CONNECTION_CLOSED.getCode()). Several overloads in the same class still build their own closed-connection exception with new DatabricksSQLException("Connection is closed", DatabricksDriverErrorCode.CONNECTION_CLOSED): prepareStatement(sql, type, concurrency, holdability), prepareCall(sql, type, concurrency, holdability) and prepareStatement(sql, autoGeneratedKeys). That constructor gets its vendor code from DatabricksVendorCode.getVendorCode(reason), which returns 0 for this message.
So a closed connection gives a different getErrorCode() (0 vs. 1010) and a different message depending on which statement-creation overload the caller uses. The PR says it standardizes on CONNECTION_CLOSED/1010, so those three inline checks should call throwExceptionIfConnectionIsClosed() too. The regression test could also cover at least one of these overloads.
(Anchored to the nearest changed line — see the description for the exact location.)
| @Override | ||
| public Statement createStatement() throws SQLException { | ||
| LOGGER.debug("public Statement createStatement()"); | ||
| throwExceptionIfConnectionIsClosed(); |
There was a problem hiding this comment.
🔵 Low — Unverified: this guard covers statement creation, but getMetaData() (around line 457) still returns new DatabricksDatabaseMetaData(this) with no closed check. Metadata calls end up reading connectionContext.isThriftNativeMetadataEnabled() or other server flags through DatabricksDriverFeatureFlagsContextFactory.getInstance(context). If they do that after close(), they could rebuild the host-scoped feature-flag cache with a closed HTTP client. That is the same failure this PR fixes. Consider adding throwExceptionIfConnectionIsClosed() to getMetaData(), or checking that the metadata path already rejects closed connections before it reads any flags.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 2 Low
Looks good: the closed-connection guards are correct and well tested, and I found no correctness problems in the main fix. There are 2 low-severity follow-ups. The closed-connection vendor code is still inconsistent across classes, and bulk setClientInfo still throws ClassCastException for non-String entries on an open connection.
| throw new DatabricksSQLException( | ||
| "Connection closed!", DatabricksDriverErrorCode.CONNECTION_CLOSED); | ||
| "Connection closed!", | ||
| DatabricksDriverErrorCode.CONNECTION_CLOSED.name(), |
There was a problem hiding this comment.
🔵 Low — Changing this helper to the (reason, sqlState, DatabricksDriverErrorCode) constructor sets the vendor code to 1010. Before, (reason, DatabricksDriverErrorCode) called DatabricksVendorCode.getVendorCode(reason), which returns 0 for "Connection closed!". The sibling closed checks were not updated, though: DatabricksDatabaseMetaData.java:1599 and DatabricksPooledConnection.java:98/206 still build CONNECTION_CLOSED with the old constructor. So a caller that runs conn.getMetaData().getTables(...) on a closed connection still gets getErrorCode() == 0, while conn.createStatement() now gets 1010. That's the same error with two different vendor codes. The changelog says CONNECTION_CLOSED/1010 is now reported "consistently". Either update those sites in the same way, or limit the changelog wording to DatabricksConnection.
| LOGGER.debug("public void setClientInfo(Properties properties)"); | ||
| throwClientInfoExceptionIfConnectionIsClosed( | ||
| properties.keySet().stream() | ||
| .filter(String.class::isInstance) |
There was a problem hiding this comment.
🔵 Low — The closed-connection pre-check filters out non-String keys so it doesn't throw. The loop just below still casts every key and value with (String) entry.getKey() / (String) entry.getValue(), though. On an open connection, a Properties with a non-String value (the new test uses ApplicationName -> 123) throws an unchecked ClassCastException instead of SQLClientInfoException, and any earlier entries are left partly applied. The new parameterized test covers non-String values only on a closed connection. Consider handling non-String entries in the loop the same way (skip them, or report them as REASON_VALUE_INVALID), so both paths give the same result.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Looks good overall. The closed-connection checks and the CONNECTION_CLOSED (1010) reporting are applied consistently, and tests cover them well. There is one medium concern: the new exception's telemetry hook can still rebuild the feature-flag cache when the connection was closed on a different thread.
There was a problem hiding this comment.
Verdict: 1 Low
Looks good overall, with one low-severity gap. Every statement factory overload now goes through the shared closed-connection check, either directly or by delegating. The setClientInfo error wrapping and the determineClientInfoStatus null-safety fix are correct, and the new tests cover them. The one gap is that statements returned by getStatement(id) aren't tracked, so they can outlive the connection.
|
|
||
| @Override | ||
| public Statement getStatement(String statementId) throws SQLException { | ||
| throwExceptionIfConnectionIsClosed(); |
There was a problem hiding this comment.
🔵 Low — The new check only rejects getStatement(id) when the connection is already closed. A statement returned by this method is never added to statementSet (compare createStatement()/prepareStatement() just below), so close() doesn't close it. Its isClosed stays false, and DatabricksStatement.checkIfClosed() only looks at the statement's own flag, never the connection's. As a result, if a caller gets a statement via getStatement(id), closes the connection, and then calls getExecutionResult(), the call still reaches the client through a closed session/HTTP client. That's the same stale-state path this PR is fixing.
Suggested fix: add the statement to statementSet, as the other factory methods do. Alternatively, have the statement's execute and result paths also check connection.isClosed().
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Medium
The closed-connection guard and the CONNECTION_CLOSED/1010 error handling look correct and well tested. One medium concern: registering handles from getStatement(statementId) in statementSet makes Connection.close() close server-side operations for async statements the connection may not own.
| throwExceptionIfConnectionIsClosed(); | ||
| DatabricksStatement statement = | ||
| new DatabricksStatement(this, StatementId.deserialize(statementId)); | ||
| statementSet.add(statement); |
There was a problem hiding this comment.
🟡 Medium — Registering the handle returned by getStatement(statementId) in statementSet changes what Connection.close() does. On close, every handle in the set gets close(false). A rehydrated handle has a non-null statementId and directResultsReceived/serverOperationClosed are both false, so the driver now sends a closeStatement RPC to the server for an operation this connection may not own.
This breaks the async pattern that getStatement(String) exists for: start executeAsync on one connection, then poll with getStatement(id).getExecutionResult() from another, often short-lived, connection (see DatabricksDriverExamples). Before this change, closing the polling connection left the server operation alone. Now it closes or cancels the query, even if it is still running or its results are still needed elsewhere. If the handle is retrieved on the connection that started the statement, close also sends a second closeStatement for the same ID.
The root cause in the PR description (stale feature-flag cache) is already fixed by the throwExceptionIfConnectionIsClosed() guard. The statementSet.add is a separate behavior change. Either drop it, or have connection close only release local resources for retrieved handles (e.g. mark them closed without the server RPC) and document the new semantics on IDatabricksConnection.getStatement.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Description
Executing a statement after closing its connection can recreate the host-scoped feature flag cache using a closed HTTP client. Later connections reuse the empty cache and fall back to legacy metadata despite the server enabling SQL Exec, causing the JDBC nightly failure.
Reject statement creation, preparation, and retrieval on closed connections using a shared check that reports
CONNECTION_CLOSED(1010). BothsetClientInfooverloads preserve the connection error and its cause, and bulk failures list only directly supplied property names. Native metadata remains server-controlled; the metadata test expectation is unchanged.Testing
pecotesting: lifecycle suite followed by METADATA-017 in the same JVM passed (13 passed, 5 expected skips). The trace confirms the native header and native result manifest after the closed-connection tests.Telemetry Errors
DatabricksDriverErrorCode.CONNECTION_CLOSED; regression assertions cover its SQLState and emitted vendor code1010.CONNECTION_CLOSEDclassification in the OSS JDBC telemetry dashboard and automation taxonomy.