Skip to content

Conversation

ruanwenjun
Copy link
Member

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.

if (isCancelled) {
return;
}
stmtHandleChangeLock.lock();
Copy link
Member

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

Copy link
Member Author

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.

@pan3793 pan3793 requested a review from turboFei July 13, 2025 19:38
@pan3793
Copy link
Member

pan3793 commented Jul 13, 2025

CI is fixed on master, you can rebase the PR to recover the CI.

@turboFei turboFei requested a review from Copilot July 13, 2025 19:45
Copilot

This comment was marked as outdated.

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fix7109 branch 2 times, most recently from a8763b6 to a6349dc Compare July 14, 2025 07:59
@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2025

Codecov Report

❌ Patch coverage is 0% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.00%. Comparing base (a1a08e7) to head (ce54a5d).
⚠️ Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
...a/org/apache/kyuubi/jdbc/hive/KyuubiStatement.java 0.00% 44 Missing ⚠️
.../org/apache/kyuubi/jdbc/hive/KyuubiConnection.java 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fix7109 branch from a6349dc to 1db3180 Compare July 14, 2025 12:30
@ruanwenjun ruanwenjun requested review from pan3793 and Copilot July 15, 2025 13:59
Copilot

This comment was marked as outdated.

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fix7109 branch from 1db3180 to c9377ac Compare July 15, 2025 14:19
@ruanwenjun ruanwenjun requested a review from Copilot July 16, 2025 09:07
Copy link

@Copilot Copilot AI left a 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 made stmtHandle, isClosed, and isCancelled volatile to prevent race conditions.
  • Wrapped cancel(), close(), and runAsyncOnServer(...) in locks to ensure atomic checks and updates.
  • Added a multithreaded test (testThrowKyuubiSQLExceptionWhenExecuteSqlOnClosedStmt) in KyuubiStatementTest 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.

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fix7109 branch from c9377ac to ce54a5d Compare July 22, 2025 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants