Skip to content

fix race condition due to premature disk handle exposure - #13919

Merged
pxLi merged 1 commit into
NVIDIA:release/25.12from
binmahone:issue_13899_on_2512
Dec 2, 2025
Merged

fix race condition due to premature disk handle exposure#13919
pxLi merged 1 commit into
NVIDIA:release/25.12from
binmahone:issue_13899_on_2512

Conversation

@binmahone

Copy link
Copy Markdown
Collaborator

This PR is to cherry pick #13900 to 25.12 branch.

… exposure

Signed-off-by: Hongbin Ma (Mahone) <mahongbin@apache.org>
@binmahone

Copy link
Copy Markdown
Collaborator Author

build

@greptile-apps

greptile-apps Bot commented Dec 2, 2025

Copy link
Copy Markdown
Contributor

Greptile Overview

Greptile Summary

Cherry-pick of PR #13900 to the 25.12 branch. Fixes a race condition in SpillableHostBufferHandle.spill() where the DiskHandle was being exposed to other threads before the DiskHandleBuilder's FileChannel was properly closed.

  • Moved staging variable declaration outside the withResource(DiskHandleStore.makeBuilder) block
  • Restructured code so the synchronized block that sets disk = staging executes after diskHandleBuilder.close() completes
  • Moved releaseHostResource() call outside the withResource block for consistency
  • The fix ensures thread-safe access to the disk file by guaranteeing the write operation completes before exposing the handle

Confidence Score: 5/5

  • This PR is safe to merge - it's a straightforward race condition fix with minimal code changes
  • Score reflects the targeted nature of the fix: only one file changed with a clear restructuring of existing code. The fix follows sound concurrency principles by ensuring the file channel is closed before exposing the disk handle. No new functionality added, just proper ordering of existing operations.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
sql-plugin/src/main/scala/com/nvidia/spark/rapids/spill/SpillFramework.scala 5/5 Fixed race condition in SpillableHostBufferHandle.spill() by moving the synchronized block that exposes the disk handle to other threads outside the withResource(DiskHandleStore.makeBuilder) block, ensuring the file channel is closed before the handle is visible.

Sequence Diagram

sequenceDiagram
    participant Thread_A as Thread A (Spilling)
    participant DiskHandleBuilder
    participant DiskHandle
    participant Thread_B as Thread B (Materializing)
    
    Note over Thread_A: Before Fix (Race Condition)
    Thread_A->>DiskHandleBuilder: write data to disk
    Thread_A->>DiskHandle: build() - creates handle
    Thread_A->>Thread_A: synchronized { disk = staging }
    Note right of Thread_A: DiskHandle exposed while<br/>builder still open!
    Thread_B->>DiskHandle: materialize() - reads file
    Note right of Thread_B: Race: FileChannel still open<br/>by builder
    Thread_A->>DiskHandleBuilder: close() - closes FileChannel
    
    Note over Thread_A: After Fix (Safe)
    Thread_A->>DiskHandleBuilder: write data to disk
    Thread_A->>DiskHandle: build() - creates handle
    Thread_A->>DiskHandleBuilder: close() - closes FileChannel
    Note right of Thread_A: withResource exits first
    Thread_A->>Thread_A: synchronized { disk = staging }
    Note right of Thread_A: Now safe to expose
    Thread_B->>DiskHandle: materialize() - reads file
    Note right of Thread_B: FileChannel properly closed
Loading

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@pxLi
pxLi merged commit b9af495 into NVIDIA:release/25.12 Dec 2, 2025
65 checks passed
@sameerz sameerz added the bug Something isn't working label Dec 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants