Skip to content

ObjectPool: synchronize mutating methods with a Mutex on threaded targets#2058

Open
AxGord wants to merge 2 commits into
openfl:developfrom
soccertutor:bugfix/objectpool-thread-safety
Open

ObjectPool: synchronize mutating methods with a Mutex on threaded targets#2058
AxGord wants to merge 2 commits into
openfl:developfrom
soccertutor:bugfix/objectpool-thread-safety

Conversation

@AxGord
Copy link
Copy Markdown

@AxGord AxGord commented May 11, 2026

Summary

lime.utils.ObjectPool mutates shared state (__pool, __inactiveObject0/1,
__inactiveObjectList, activeObjects, inactiveObjects) from get /
release / add / remove / clear / size-setter without
synchronization. When two threads drive the same pool concurrently, the
counters drift and the same instance can be handed out twice.

This PR adds a sys.thread.Mutex guarded behind #if target.threaded so
single-threaded targets (js, flash) keep the existing zero-overhead path,
and adds unit tests covering both the regular API surface and the
multi-thread regression.

The bug

Both get() and release() decrement-and-increment activeObjects /
inactiveObjects and reassign __inactiveObject0/1 without locking.
Classic interleaving:

  • Thread A reads inactiveObjects > 0 → true, enters __getInactive().
  • Thread B reads inactiveObjects > 0 → still true (A hasn't decremented yet),
    also enters __getInactive().
  • Both threads pull __inactiveObject0, the same instance is returned to
    both callers; or inactiveObjects is double-decremented and ends up negative.

Fix

  • Add sys.thread.Mutex __mutex field under #if target.threaded.
  • Wrap the mutating public methods (add, clear, get, release,
    remove, set_size) with two inline helpers __lock() / __unlock()
    that compile to no-ops on non-threaded targets.
  • Internal helpers __addInactive / __getInactive / __removeInactive
    are only called from already-locked public paths — they don't lock again.

One small behaviour change

In #if debug mode release() now also calls __unlock(); return; after
each Log.error so a release with an invalid object can't continue into
activeObjects-- and __pool.remove(object). Without the early return,
the function continued and corrupted pool state further past the detection.
Only observable in debug builds.

Tests

New tests/unit/src/utils/ObjectPoolTest.hx (registered in TestMain.hx):

  • Eight single-threaded functional tests: get/release reuse, size cap,
    clean callback, clear, remove, size-setter pre-fill.
  • One multi-thread regression testConcurrentGetReleaseDoesNotDriftCounters
    guarded by #if target.threaded: 8 worker threads × 1000 get/release
    iterations on a 4-slot pool, with a 5-second deadline in the waiter
    loop so the test fails cleanly if pool state corrupts (no CI hang).

Result on neko (un-mutexed run is from develop, run via temporary checkout):

Variant Counters after run testConcurrentGetReleaseDoesNotDriftCounters
unpatched active=5, inactive=-1 (typical) FAIL (counter drift)
patched active=0, inactive=4 (every run) PASS

Total: 9 tests, 23 assertations, ~11 ms on neko in both builds.

Risk

  • Non-threaded targets (js, flash) — zero runtime cost. __lock() /
    __unlock() are inline and their bodies are #if target.threaded-gated,
    so every call site (get, release, add, clear, remove, set_size)
    compiles away completely. Verified by inspecting generated JS: no
    __lock / __unlock / __mutex references at any call site. Two
    empty function definitions remain in the output (~40 bytes of dead code,
    never invoked) — happy to #if-gate those away too if reviewers prefer
    zero bytes.

  • Threaded targets — single-thread use pays an uncontended-lock cost.
    Benchmarked on neko (5M get/release iterations, single thread, 64-slot
    pool): unpatched ~330 ns per get/release pair, patched ~565 ns — about
    +70% on neko, where Mutex is heavy. On cpp/hl with native
    uncontended mutex (std::mutex / pthread futex) the cost is typically
    in the low-tens-of-ns per acquire/release, so the percent overhead is
    much smaller. In real OpenFL hot paths (Event pool, Graphics
    shaderBuffer pool, etc.) the call rate is ~tens-to-hundreds per frame,
    so the absolute cost stays in the microsecond range per frame even on
    neko.

  • Multi-thread use is the entire point of the PR. Without the mutex,
    the test above demonstrates the pool ends up with inactiveObjects=-1
    and hands out duplicates; the patched version is stable.

  • If reviewers want to keep single-thread users opt-out, I'm happy to
    add a -D lime_objectpool_no_mutex compile flag in a follow-up.

@joshtynjala
Copy link
Copy Markdown
Member

I wonder if it would make sense to add a separate ThreadSafeObjectPool.

AxGord and others added 2 commits May 14, 2026 02:58
ObjectPool mutates shared state (counters, fast slots, the inactive list)
from get / release / add / remove / clear / size-setter. On threaded
targets, concurrent use causes counter drift and duplicate handouts.

ThreadSafeObjectPool extends ObjectPool and overrides every mutating
public method to wrap the super call with a sys.thread.Mutex
acquire / release pair. The non-mutating get_size accessor and the
dynamic create / clean callbacks are intentionally untouched (callbacks
are invoked from already-locked paths).

On non-threaded targets (js, flash) the class is replaced with a
'typedef ThreadSafeObjectPool<T> = ObjectPool<T>'. Same compile-time
API, zero runtime cost, no thread-safety code emitted.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ObjectPoolTest covers the single-threaded API surface: get/release reuse,
size cap, clean callback, clear, remove, size-setter pre-fill.

ThreadSafeObjectPoolTest, guarded by '#if target.threaded', adds a
concurrent regression: 8 worker threads x 1000 get/release iterations on
a 4-slot pool, with a 5-second deadline so the test fails cleanly if
pool state corrupts. Verifies no duplicate handouts and counters return
to expected values.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@AxGord AxGord force-pushed the bugfix/objectpool-thread-safety branch from 85591ef to c8266f1 Compare May 13, 2026 23:59
@AxGord
Copy link
Copy Markdown
Author

AxGord commented May 14, 2026

Done — pushed c8266f15 (force-push, the original two commits were replaced).

Design now:

  • lime.utils.ThreadSafeObjectPool<T> extends ObjectPool<T> under #if target.threaded, overrides every mutating public method (add, clear, get, release, remove, set_size) to wrap the super call with Mutex.acquire() / release().
  • On non-threaded targets (js, flash) the class is replaced with typedef ThreadSafeObjectPool<T> = ObjectPool<T> — zero runtime cost, no thread-safety code emitted.
  • ObjectPool itself is reverted to the develop state (no mutex, no behaviour change for existing callers).
  • Tests split: ObjectPoolTest keeps the eight single-threaded cases, new ThreadSafeObjectPoolTest holds the concurrent regression under #if target.threaded.

Class doc-comment notes two caveats: create / clean callbacks run while the mutex is held (no reentry into the same pool), and activeObjects / inactiveObjects / size reads from another thread are not memory-safe.

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.

2 participants