Re: [PR] [KYUUBI #7290] Prevent engine session leak after Kyuubi session closed [kyuubi]

["Copilot (via GitHub)" <gi...@apache.org>]

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: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org