Repository navigation
fix: share one close across concurrent session and client close() calls - #607
vuanhphung wants to merge 2 commits into
Conversation
DBSQLSession.close() marked the session closed only after the CloseSession RPC returned, so concurrent callers each sent CloseSession for the same handle. DBSQLClient.close() had the same race and released the shared telemetry client once per caller. Both now return the in-flight close to concurrent callers. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
![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
Mostly good: the shared in-flight close removes the duplicate CloseSession / telemetry-release race, and the tests cover it. One medium concern: the session's shared close promise is never cleared, so if closing one of its operations fails, that error is cached and the session can never be closed or retried.
The shared session close was never cleared, so if closing the session's operations rejected before CloseSession was sent, every later close() returned the same rejection and the session could not be closed. Clear the shared close once it settles, as DBSQLClient.close() already does. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
|
The red This comment was written by Isaac. |
Problem
DBSQLSession.close()setsisOpen = falseonly after theCloseSessionRPC returns, so concurrent callers all pass the open check and each sendCloseSessionfor the same handle.DBSQLClient.close()has the same race: each concurrent call closes the sessions and releases the sharedTelemetryClientrefcount (TelemetryClientProvider.releaseClient), which can close the telemetry client still used by otherDBSQLClients on the same host.Found by the cross-driver session-lifecycle spec (SESSION-014, Concurrent Session Close: concurrent disposes must not throw and must send exactly one
CloseSession). A live run againstmainthrough the test proxy showed 3CloseSessionrequests forPromise.all([client.close(), client.close(), client.close()])on Thrift. The kernel backend already sends oneDeleteSession.Change
close()on a session and on a client now returns the in-flight close to concurrent callers, so the work runs once and every caller gets the same result or error. The shared close is cleared when it settles, so a close that failed beforeCloseSession(for example an operation failed to close) can be retried, and a reconnected client can be closed again.Verification
npm test(1755 passing),npm run lint,npm run prettieron the changed files.closeSessionfor concurrent session closes, a shared rejection when it fails, a retry after closing operations failed, and oneCloseSession/ backend close / telemetry release for concurrent client closes. Each fails without the change.CloseSessionand its xfail tripwire throwsXPASS; the other session-lifecycle tests still pass on Thrift and SEA.This pull request and its description were written by Isaac.