-
Notifications
You must be signed in to change notification settings - Fork 962
[KYUUBI #7133] Use stmtHandleChangeLock in KyuubiStatement to ensure thread safe access and updates to stmtHandle. #7134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
if (isCancelled) { | ||
return; | ||
} | ||
stmtHandleChangeLock.lock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should check isCancelled again after obtaining the lock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I’ve moved the check after stmtHandleChangeLock.lock() — this should look good now.
CI is fixed on master, you can rebase the PR to recover the CI. |
…ess and updates to stmtHandle.
a8763b6
to
a6349dc
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7134 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 700 701 +1
Lines 43454 43506 +52
Branches 5883 5896 +13
======================================
- Misses 43454 43506 +52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a6349dc
to
1db3180
Compare
1db3180
to
c9377ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces thread-safety controls around the statement handle in KyuubiStatement and adds a concurrent unit test to verify behavior when closing and executing simultaneously.
- Added a
ReentrantLock
(stmtHandleAccessLock
) and madestmtHandle
,isClosed
, andisCancelled
volatile to prevent race conditions. - Wrapped
cancel()
,close()
, andrunAsyncOnServer(...)
in locks to ensure atomic checks and updates. - Added a multithreaded test (
testThrowKyuubiSQLExceptionWhenExecuteSqlOnClosedStmt
) inKyuubiStatementTest
to assert proper exception on execute after close.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/KyuubiStatement.java | Introduced stmtHandleAccessLock and volatile flags; applied locking in key methods to ensure thread safety. |
kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/KyuubiConnection.java | Made the isClosed flag volatile for safe concurrent reads. |
kyuubi-hive-jdbc/src/test/java/org/apache/kyuubi/jdbc/hive/KyuubiStatementTest.java | Added a concurrent test to validate exception is thrown when executing after close. |
kyuubi-hive-jdbc/src/test/java/org/apache/kyuubi/jdbc/hive/KyuubiStatementTest.java
Outdated
Show resolved
Hide resolved
kyuubi-hive-jdbc/src/test/java/org/apache/kyuubi/jdbc/hive/KyuubiStatementTest.java
Outdated
Show resolved
Hide resolved
kyuubi-hive-jdbc/src/test/java/org/apache/kyuubi/jdbc/hive/KyuubiStatementTest.java
Outdated
Show resolved
Hide resolved
kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/KyuubiStatement.java
Show resolved
Hide resolved
c9377ac
to
ce54a5d
Compare
Why are the changes needed?
The check for null and the subsequent use or assignment of the stmtHandle is not an atomic operation, might cause the stmt doesn't be closed.
How was this patch tested?
UT.
Was this patch authored or co-authored using generative AI tooling?
NO.