Skip to content

Fix race condition in ExternalTsFileQueryResource reference counting - #18371

Merged
shuwenwei merged 1 commit into
masterfrom
fix-external-tsfile-resource-race
Jul 31, 2026
Merged

Fix race condition in ExternalTsFileQueryResource reference counting#18371
shuwenwei merged 1 commit into
masterfrom
fix-external-tsfile-resource-race

Conversation

@shuwenwei

Copy link
Copy Markdown
Member

Problem

When multiple FragmentInstances share an ExternalTsFileQueryResource, a race condition can cause the resource to be closed prematurely, resulting in "ExternalTsFileQueryResource has been closed" exceptions.

Trigger conditions:

  1. Query uses read_tsfile(...), multiple Fragments share one ExternalTsFileQueryResource
  2. Many devices/timeseries, IoTDB splits into multiple Fragments by parallelism (e.g. PARTITION BY timeseries_id)
  3. Fragment startup is not synchronized:
    • Fragment A initializes first, reference count = 1
    • Fragment B is still in the scheduling queue, hasn't called retain()
  4. Fragment A finishes quickly and releases its reference, count becomes 0 → resource closed
  5. Fragment B then initializes and calls retainFragmentInstanceUsage() → throws "ExternalTsFileQueryResource has been closed"

Another path: QueryExecution cleanup runs while no FragmentInstance has initialized yet (count=0), closing the resource before scheduled FIs start.

Easier to trigger with:

  • Large number of timeseries/devices, producing many partitions
  • DataNode parallelism > 1
  • Uneven partition sizes (some Fragments complete quickly, others start late)
  • Many concurrent queries, Drivers queued in thread pool
  • High CPU/IO pressure, widening Fragment startup time gap
  • Query cancellation/error overlapping with Fragment scheduling

Fix

Introduce a two-phase close mechanism:

  • closeByFragmentInstance(): only decrements the usage count. It no longer closes the resource on its own — it waits for the QueryExecution signal.
  • closeByQueryExecution(): sets a wantsClose flag and closes only when the usage count reaches zero.
  • QueryExecution state listener: now calls releaseExternalTsFileQueryResources() for all terminal states (including FINISHED), not just error states, ensuring the resource is always eventually closed.

Changes:

File Change
ExternalTsFileQueryResource.java Add queryExecutionWantsToClose flag; modify closeByFragmentInstance and closeByQueryExecution close conditions
QueryExecution.java Call releaseExternalTsFileQueryResources() for all terminal states

Why this approach:

  • No blocking: pure flag mechanism, no wait/notify
  • Retry-safe: retry creates new Resource objects, old ones close normally via existing FI releases
  • No leaks: FI failures that never call retain() don't affect the count, unlike a total-count approach
  • Backward compatible: single-Fragment behavior unchanged; closeByQE is idempotent on repeated calls

When multiple FragmentInstances share an ExternalTsFileQueryResource, a race
condition can cause the resource to be closed prematurely:

1. Fragment A initializes (retain, count=1) and finishes quickly,
   releasing its reference (count=0) which triggers close().
2. Fragment B is still in the scheduling queue and has not called retain()
   yet. When it eventually calls retain(), it fails with
   'ExternalTsFileQueryResource has been closed'.

Another path: QueryExecution cleanup runs while no FragmentInstance has
initialized yet (count=0), closing the resource before scheduled FIs start.

Fix: introduce a two-phase close mechanism.

- closeByFragmentInstance(): only decrements the usage count. It no longer
  closes the resource on its own — it must wait for the QueryExecution signal.
- closeByQueryExecution(): sets a wantsClose flag and closes only when the
  usage count is zero.
- QueryExecution state listener: now calls releaseExternalTsFileQueryResources()
  for all terminal states (including FINISHED), not just error states, ensuring
  the resource is always eventually closed.
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 43.28%. Comparing base (1546441) to head (dcb71a2).

Files with missing lines Patch % Lines
...n/tvf/read_tsfile/ExternalTsFileQueryResource.java 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #18371      +/-   ##
============================================
- Coverage     43.45%   43.28%   -0.17%     
  Complexity      374      374              
============================================
  Files          5364     5364              
  Lines        383165   383167       +2     
  Branches      49849    49850       +1     
============================================
- Hits         166516   165866     -650     
- Misses       216649   217301     +652     

☔ View full report in Codecov by Harness.
📢 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.

@ColinLeeo
ColinLeeo self-requested a review July 31, 2026 09:06
@shuwenwei
shuwenwei merged commit 75eaa40 into master Jul 31, 2026
21 of 23 checks passed
@shuwenwei
shuwenwei deleted the fix-external-tsfile-resource-race branch July 31, 2026 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants