#154: ThreadPool/ThreadChannel — $this/method-as-closure coverage + fix worker crash on exit()/die()#155
Merged
Conversation
…ootloader errors, add $this coverage
Worker-level fixes in thread_pool.c:
- exit()/die() (unwind-exit token) or a fatal error (bailout) in a task or the
bootloader crashed the worker fiber ("Error transfer requires a throwable
value" assert + ASAN UAF at zend_fibers.c:491). The worker now runs the call
under zend_try and checks zend_is_unwind_exit/zend_is_graceful_exit, converts
it into an Async\ThreadTransferException delivered to awaiters, and exits
cleanly instead of re-raising zend_bailout() into the fiber.
- A $this-bound bootloader that can't load on the worker (or whose body threw)
had its real error turned into an E_WARNING and discarded, with tasks rejected
by a generic "cancelled before execution". thread_pool_drain_tasks gained a
reject_with parameter so the actual cause now reaches every awaiter.
Tests:
- thread_pool/042-059, thread_channel/038-039: full $this + method-as-closure
transfer matrix for submit()/send() (was only covered for spawn_thread).
- thread_pool/060,063: bootloader transfer-error / thrown-exception propagation.
- thread_pool/061,062: exit()/die() in bootloader / sync task -> clean
ThreadTransferException, no crash.
Known follow-up: coroutine-mode task exit() resolves the future to null
(graceful success); surfacing worker-exit separately is tracked for respawn work.
…er keeps serving Follow-up to the crash fix: a graceful exit()/die() in a task is "this task is done", not a fatal — the worker's request survives it. Empirically a single worker keeps serving the next task after one exits (verified mixing exit() with throwing and normal tasks). So a task exit() now resolves that task's future to null and the worker continues, instead of delivering ThreadTransferException and tearing the pool down. No respawn machinery is needed since the worker never actually dies. A real fatal (zend_bailout, e.g. OOM) still tears the pool down via ThreadTransferException — the request is unusable. Bootloader exit() likewise remains a hard pool failure (setup did not complete). Reverted the coroutine-mode dispose unwind-token guard: coroutine-mode exit already resolves the future to null through the normal path, so the guard was dormant and inconsistent with the null model. Tests: 062 now asserts exit -> null + next task runs; new 064 stresses a single worker with exit/throw/normal interleaved.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
…rror CI (LINUX_X64_DEBUG_ZTS, non-ASAN) caught a SEGV in 060: the bootloader transfer-failed path deep-copied the raw 'Cannot load transferred object' Error across the thread boundary to the awaiter. That Error is thrown from inside the cross-thread transfer machinery and its backtrace reaches into worker-local load state, so loading it on the parent thread crashes (race, only reproducible under real multi-core scheduling — ASAN's heap layout masked it). Now we re-ship only the message as a clean ThreadTransferException, matching the bailout and spawn_thread paths. 060 green 10/10 on non-ASAN ZTS.
EdmondDantes
deleted the
154-threadpoolthreadchannel-no-test-coverage-for-transferring-this-bound-closures-incl-method-as-closure
branch
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.
Closes #154.
Summary
Two real fixes plus the missing
$thistest coverage that started the investigation.Fixes (
thread_pool.c)exit()/die()in a task or bootloader. A gracefulexit()/die()threw an unwind-exit token that the worker either passed toreject()or re-raised viazend_bailout()— both crash the worker fiber ("Error transfer requires a throwable value", assert + ASAN UAF atzend_fibers.c:491). The worker now runs the task/bootloader call underzend_tryand inspectszend_is_unwind_exit()/zend_is_graceful_exit():exit()/die()in a task is graceful "this task is done" — the worker's request survives, so the future resolves tonulland the worker keeps serving the next task (no respawn needed; verified mixingexit()/throw/normal on one worker).zend_bailout) orexit()/die()in the bootloader delivers anAsync\ThreadTransferExceptionto pending awaiters and tears the pool down.$this-bound bootloader transfer error was swallowed. When a bootloader bound to$thisof a class missing on the worker (or whose body threw) failed, the real error became anE_WARNINGand tasks were rejected with a generic cancellation.thread_pool_drain_tasksgained areject_withparameter so the actual cause now reaches every awaiter.Test coverage
The
$this/ method-as-closure transfer matrix existed only forspawn_thread. Added it forThreadPool::submit()andThreadChannel::send():thread_pool/042–059,thread_channel/038–039: base, private/protected, self-cycle, readonly, enum (instance + property), WeakReference, typed props, method dispatch, first-class callable ($this->m(...),$obj->m(...)),Closure::fromCallable,Closure::bind, arrow-fn, nested closure, static first-class, missing-class error,$this+use().thread_pool/060,063: bootloader transfer-error / thrown-exception propagation.thread_pool/061,062,064:exit()/die()in bootloader / task — clean behaviour, no crash, worker survives.Testing
Full
tests/thread*,tests/thread_pool,tests/thread_channelsuites green under ZTS + ASAN/UBSAN (174 passed, 0 failed).Notes
exit()already resolves the future tonullvia the normal path — consistent with the new sync-mode behaviour.ext/asynconly.