Add optional compile-time thread-safety for the forwarding API#179
Merged
Conversation
The `lbt_*` mutation API (`lbt_forward()`, `lbt_set_forward()`, `lbt_set_forward_by_index()`) is documented as thread-unsafe: it mutates global forwarding tables with no locking. This adds an opt-in process-global lock, enabled with `make LBT_THREADSAFE=1`, that serializes those mutators so racing initializers cannot corrupt the tables. - New `lbt_lock()`/`lbt_unlock()` helpers: a `pthread_mutex_t` (static initializer) elsewhere, a `CRITICAL_SECTION` (initialized in `DllMain`) on Windows. When `LBT_THREADSAFE` is undefined they compile to no-ops, so the default build is byte-for-byte equivalent in behavior to before. - The public mutators now wrap unlocked `*_impl` workers; internal callers (`lbt_forward_impl`, the constructor) call the workers directly to avoid re-entrant locking. - `Make.inc` gains the `LBT_THREADSAFE` toggle (default 0), adding `-pthread` on non-Windows when enabled. Scope note: only the mutators are locked; read-only accessors (`lbt_get_config()`, `lbt_get_forward()`) remain unlocked, so callers must still avoid racing reads against concurrent reconfiguration. Verified: builds with and without the flag; public symbols still exported and the `*_impl` workers stay internal; a dlopen smoke test exercises the lock path without deadlock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The locking added in this PR was previously never compiled or run by the tests: `build_libblastrampoline()` always built the default, no-op build. Make the harness build with the optional internal locking compiled in by default, so every backend's tests run against a `-DLBT_THREADSAFE` library and each `lbt_forward()`/`lbt_set_forward()` goes through `lbt_lock()`/`lbt_unlock()` across the whole existing CI matrix (all OSes/interfaces). The value is read from the `LBT_THREADSAFE` env var, so `LBT_THREADSAFE=0` still tests the plain build that ships by default. Verified locally: the `direct` backend passes (74/74) with the library built `-DLBT_THREADSAFE -pthread`, both with the env var set and via the new default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mention `make LBT_THREADSAFE=1` in the thread-safety note: it compiles in a process-global lock around the mutating API (pthread mutex / CRITICAL_SECTION), is off by default, and guards mutators only (readers and call forwarding stay lock-free). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ad-safety # Conflicts: # src/libblastrampoline.c
Promote the thread-safety note to a `### Threading` section and trim it to the essentials: thread-unsafe by default, `make LBT_THREADSAFE=1` adds a lock around the mutating API, readers/forwarding stay lock-free. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The
lbt_*mutation API (lbt_forward(),lbt_set_forward(),lbt_set_forward_by_index()) is documented as thread-unsafe — it mutates global forwarding tables with no locking, and the README explicitly warns against loading BLAS libraries from two threads at once. This adds an opt-in process-global lock so that the common "configure once at startup from racing initializers" case is safe.Design
lbt_lock()/lbt_unlock()helpers:pthread_mutex_twith a static initializerCRITICAL_SECTIONinitialized inDllMain(DLL_PROCESS_ATTACH)LBT_THREADSAFEis undefined, both compile to no-ops, so the default build is behaviorally identical to today.*_implworkers. Internal callers (lbt_forward_impl, the startup constructor) call the workers directly, so there's no re-entrant locking / deadlock.Make.incgains anLBT_THREADSAFEtoggle (default0); when1it adds-DLBT_THREADSAFEand-pthread(non-Windows).Scope
Only the mutators are locked. Read-only accessors (
lbt_get_config(),lbt_get_forward()) are intentionally left unlocked, so callers must still avoid racing reads against concurrent reconfiguration. (Happy to extend coverage if reviewers prefer — that's partly why this is a draft.)Verification
LBT_THREADSAFE=1.*_implworkers stay internal (not innm -D).dlopensmoke test calls the locked entry points repeatedly without deadlocking (proves lock acquire/release).Draft for review.