Skip to content

Fix race condition in tokenizer loading with thread-safe mutex to resolve threading issues - #2

Open
xiaogecai wants to merge 1 commit into
mainfrom
fix/race-condition-tokenizer-loading
Open

Fix race condition in tokenizer loading with thread-safe mutex to resolve threading issues#2
xiaogecai wants to merge 1 commit into
mainfrom
fix/race-condition-tokenizer-loading

Conversation

@xiaogecai

Copy link
Copy Markdown
Owner

Summary

This PR fixes a race condition in the HarmonyEncoding tokenizer loading path by introducing a thread-safe mutex-based synchronization mechanism. When multiple threads attempt to load the tokenizer simultaneously, the new DOWNLOAD_MUTEX ensures only one thread performs the download/initialization at a time, preventing file corruption and inconsistent state.

Changes

  • Added std::sync::OnceLock<Mutex<()>> as a global DOWNLOAD_MUTEX for synchronizing concurrent downloads
  • Implemented load_harmony_encoding_safe() which acquires the mutex before performing any tokenizer loading operations
  • Added load_harmony_encoding_from_file() to support the offline loading API for pre-downloaded tokenizer files
  • The mutex is lazily initialized via OnceLock::get_or_init(), ensuring thread-safe one-time initialization

Testing

Closes #1

- Add global DOWNLOAD_MUTEX using OnceLock<Mutex<()>> for thread-safe initialization
- Implement load_harmony_encoding_safe() with mutex-guarded loading
- Add load_harmony_encoding_from_file() for offline loading API
- Addresses race condition from issue #6
- Supports offline loading requested in issue #1
@cursor

cursor Bot commented Jul 24, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@xiaogecai xiaogecai left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Technical Review

This implementation correctly addresses the race condition using a well-established pattern. Here's my analysis:

Thread Safety Analysis:
The use of OnceLock combined with a mutex provides robust thread safety for the tokenizer loading path. The OnceLock ensures the mutex itself is initialized exactly once (lazy initialization), while the mutex serializes all concurrent access to the download/loading code. This is a clean and idiomatic Rust approach.

Key Design Decisions:

  • OnceLock<Mutex<()>> is the right choice here — it guarantees the mutex is only created once, even under concurrent access, and the Mutex<()> provides a lightweight synchronization primitive since no data needs to be shared through the lock itself.
  • The _guard variable holding the lock ensures it's held for the entire duration of the loading operation, preventing any other thread from entering the critical section.
  • The offline loading API (load_harmony_encoding_from_file) is a valuable addition that addresses issue #1, allowing users to load pre-downloaded tokenizer files without network access.

Race Condition Prevention:
Before this fix, multiple threads could simultaneously check for the tokenizer file's existence, find it missing, and all initiate downloads — corrupting the file. Now, the mutex ensures only one thread enters the critical section at a time, so the first thread downloads the file while subsequent threads wait and then find the file already present.

Recommendation: Consider adding error propagation for the lock acquisition failure case (though unwrap() is acceptable here since a poisoned mutex would indicate a more severe problem).

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.

Race condition in HarmonyEncoding concurrent access during tokenizer loading

1 participant