What is missing
ThreadPool has no liveness tracking, so a worker thread that dies cannot be observed by any caller.
getWorkerCount() (thread_pool.c:1764-1769) returns pool->base.worker_count, a plain non-atomic int32_t written only in thread_pool_create (lines 1162, 1194, 1200). Nothing decrements it. It is a construction-time constant.isClosed() reads pool->base.closed, which only explicit close()/cancel() flips. A dying worker never touches it.pool->base.workers[], the OS thread handle array, is write-only: allocated at 1190, written at 901, freed at 1337-1339, and read nowhere in the extension.- The worker's own exit path (
thread_pool.c:726-729) does a refcount release and frees its context. It records nothing.
So a pool that has lost half its threads still answers with the number it was constructed with, and submit() keeps accepting work whose Futures will never settle.
Where it already bites
reload() takes the cohort size from that same stale count (thread_pool.c:995) and sizes its exit-token channel from it (line 1019), so after a worker death it waits for tokens from threads that no longer exist. Measured: reload() hangs, while the same call on a pool that never lost a worker returns normally.
This was found while chasing #229, where an exception escaping a coroutine-mode task destroyed the worker that ran it. That path is fixed, but the silence around it is not: any future cause of a worker death — a bailout, an OOM, a bug in a bootloader — is equally invisible.
There is a hint that this was noticed once before: tests/thread_pool/070-respawn_worker.php exists in the working tree (gitignored) with a stale .out reading Call to undefined method Async\ThreadPool::respawnWorker().
What would close it
Two separable pieces:
- Observation. Mark the slot dead when
thread_pool_worker_handler returns, and expose a count of live workers. worker_count would have to become atomic, since it would then be read from PHP-land while workers exit on other threads.
- Recovery. Optionally respawn, or at least fail
submit() loudly once no worker remains, instead of returning a Future that never settles.
Observation alone is worth having: today no test can express "no worker died", because the value that would say so does not exist. That is precisely why #229 went unnoticed — its tests asserted that the Future rejected, which was always true, while the pool bled threads.
What is missing
ThreadPoolhas no liveness tracking, so a worker thread that dies cannot be observed by any caller.getWorkerCount()(thread_pool.c:1764-1769) returnspool->base.worker_count, a plain non-atomicint32_twritten only inthread_pool_create(lines 1162, 1194, 1200). Nothing decrements it. It is a construction-time constant.isClosed()readspool->base.closed, which only explicitclose()/cancel()flips. A dying worker never touches it.pool->base.workers[], the OS thread handle array, is write-only: allocated at 1190, written at 901, freed at 1337-1339, and read nowhere in the extension.thread_pool.c:726-729) does a refcount release and frees its context. It records nothing.So a pool that has lost half its threads still answers with the number it was constructed with, and
submit()keeps accepting work whose Futures will never settle.Where it already bites
reload()takes the cohort size from that same stale count (thread_pool.c:995) and sizes its exit-token channel from it (line 1019), so after a worker death it waits for tokens from threads that no longer exist. Measured:reload()hangs, while the same call on a pool that never lost a worker returns normally.This was found while chasing #229, where an exception escaping a coroutine-mode task destroyed the worker that ran it. That path is fixed, but the silence around it is not: any future cause of a worker death — a bailout, an OOM, a bug in a bootloader — is equally invisible.
There is a hint that this was noticed once before:
tests/thread_pool/070-respawn_worker.phpexists in the working tree (gitignored) with a stale.outreadingCall to undefined method Async\ThreadPool::respawnWorker().What would close it
Two separable pieces:
thread_pool_worker_handlerreturns, and expose a count of live workers.worker_countwould have to become atomic, since it would then be read from PHP-land while workers exit on other threads.submit()loudly once no worker remains, instead of returning a Future that never settles.Observation alone is worth having: today no test can express "no worker died", because the value that would say so does not exist. That is precisely why #229 went unnoticed — its tests asserted that the Future rejected, which was always true, while the pool bled threads.