Copilot commented on code in PR #7294:
URL: https://github.com/apache/kyuubi/pull/7294#discussion_r2658730637
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/session/KyuubiSessionImpl.scala:
##########
@@ -180,6 +180,11 @@ class KyuubiSessionImpl(
_engineSessionHandle =
engineClient.openSession(protocol, user, passwd,
openEngineSessionConf)
_client = engineClient
+ // Since the _client might be null when we close session
+ // so we need to check isClosed here
Review Comment:
The comment is misleading. The issue is not that "_client might be null when
we close session", but rather that the Kyuubi session might have been closed
(by another thread) while the engine session was being initialized. Consider
revising to: "Check if the Kyuubi session was closed while opening the engine
session"
```suggestion
// Check if the Kyuubi session was closed while opening the
engine session
```
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/session/KyuubiSessionImpl.scala:
##########
@@ -180,6 +180,11 @@ class KyuubiSessionImpl(
_engineSessionHandle =
engineClient.openSession(protocol, user, passwd,
openEngineSessionConf)
_client = engineClient
+ // Since the _client might be null when we close session
+ // so we need to check isClosed here
+ if (isClosed) {
+ throw KyuubiSQLException(s"KyuubiSession $handle has been
closed")
+ }
Review Comment:
This critical race condition fix lacks automated test coverage. Consider
adding a test that simulates closing a Kyuubi session while the engine session
is being initialized to verify that the engine session is properly cleaned up.
This could be done by using a mock or by introducing a delay in the engine
session opening process during the test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
