- 
                Notifications
    You must be signed in to change notification settings 
- Fork 967
[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, andisCancelledvolatile to prevent race conditions.
- Wrapped cancel(),close(), andrunAsyncOnServer(...)in locks to ensure atomic checks and updates.
- Added a multithreaded test (testThrowKyuubiSQLExceptionWhenExecuteSqlOnClosedStmt) inKyuubiStatementTestto 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 stmtHandleAccessLockand 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 isClosedflag 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.