Conversation
Member
|
This PR seems to have accumulated a bunch of extra work around the |
ngunyimacharia
force-pushed
the
fix/async-task-symfony8
branch
from
September 2, 2026 12:25
baf8020 to
1177494
Compare
ngunyimacharia
force-pushed
the
fix/async-task-symfony8
branch
from
September 2, 2026 12:28
1177494 to
200225b
Compare
Contributor
Author
|
@simonhamp fixed. Separate PR created. |
SRWieZ
approved these changes
Sep 4, 2026
simonhamp
approved these changes
Sep 18, 2026
simonhamp
added a commit
that referenced
this pull request
Sep 18, 2026
…228) * feat: async tasks — background PHP work with UI completion callbacks Adds AsyncTask::dispatch(static fn () => ...) (and a $this->async() shorthand on NativeComponent) to run work on a background PHP thread and handle the result back on the UI thread, where callbacks can mutate component state. Builds on two things that already existed: the background-interpreter lanes (worker/ephemeral/webview) and the native event channel that already delivers camera results into a parked runloop. This wires them together and adds a dedicated, concurrent async lane. - Work closure must be static; a $this-bound closure is rejected by reflection at dispatch time rather than failing silently in a background log. - finished()/failed() are rebound to the live component. They are screen-scoped and dropped if the user navigated away; shared('alias') opts out by delivering a named event any active screen can handle via #[On]. - Runs immediately and concurrently on its own pool of PHP contexts — not the queue worker, no SQLite, no queue, one attempt, no retries. - Payload crosses via a temp file, so native stays a courier with two bridge functions: AsyncTask.Dispatch and AsyncTask.Complete. - Jump runs tasks in a dev-machine subprocess and drains a completion spool. - AsyncTask::fake() runs work inline for tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * style: satisfy Pint's fully_qualified_strict_types on the async task files CI's Code Style job flagged 8 files. Each contained a fully-qualified class reference where the class was imported or in the same namespace, which is exactly what the fully_qualified_strict_types fixer rewrites. - Docblock FQCNs replaced with imported short names. - Inline FQCNs in tests replaced with imports (AsyncTask::clearFake(), SerializableClosure). - NativeComponent: dropped the `use Closure;` added for async(). That import retroactively made the file's ten pre-existing `\Closure` usages shortenable, so the signature now uses `\Closure` to match the file's own convention — a two-line diff instead of churning unrelated lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Close the paths where an async task could deliver nothing Review feedback on #228. The PHP layer was sound; the gaps were in the native lanes and in the failure paths that silently delivered nothing — the worst outcome this API can have, since the UI is sitting on a spinner waiting for a callback that never arrives. Hot reload / shutdown safety: - Android `AsyncTaskExecutor.stop()` now joins its pool (and the new watchdog thread) before returning, like `PHPQueueWorker.stop()`, and reports whether it drained. Callers stop it immediately before `shutdownPersistentRuntime()`, which frees Zend state a live async context still references — returning early tore PHP down underneath a running task. - iOS `stop()` no longer bails when the pool is mid-boot. It signals the boot loop to stop, waits it out, then tears down every slot it finds. Bailing left the C slots allocated with `start()` refusing to run twice, so every later dispatch was dropped for the life of the app. Concurrency: - Dropped the `setenv("APP_RUNNING_IN_CONSOLE"/"PHP_SELF")` calls from the async lane in both `php_bridge.c` and `PHP.c`. Four pool threads flipping a process-wide, non-thread-safe env var clobber each other and the UI lane. The eval'd code sets the per-thread `$_SERVER` entries instead — which is what Laravel's `Env` reads — including before the bootstrap runs. Every dispatch now reaches exactly one outcome: - `AsyncTask.Dispatch`'s `success` flag is checked. No executor, no slot, no bridge, or a Jump subprocess that wouldn't launch fails the dispatch at the call site and fires `->failed()`. - The result is JSON round-tripped in the background context (`AsyncTaskRunner::normalizeResult()`), where a non-encodable value can still be reported as an ordinary task failure. `encodeCompletion()` backstops the envelope itself. - Every dispatch carries a deadline (default 60s, `->timeout($seconds)`, 0 to disable) plus the pre-built completion to post if it passes, so the native watchdogs stay dumb couriers. Jump sweeps overdue subprocesses on each runloop tick instead. The work isn't killed — the timeout unblocks the UI, and a late completion for an already-failed task is discarded. Smaller ones from the same review: - Screen scoping holds the origin as a `WeakReference`, closing the `spl_object_id` reuse hole where a popped screen's id could be handed to the next component and deliver its callback to an unrelated screen. - `$nativeActiveComponent` is weak and restored when a runloop exits, so it stops retaining a dead component. - `AsyncTask::fake()` runs the same JSON normalization as the device, so a test can't pass on a value a device would never deliver. - `__destruct()` can no longer throw; a start failure reports through the task's own `failed()` channel and the error log. - Async callbacks register with `durable: false`, so the "never touches SQLite" claim is now true — `NativeCallbacks::register()` was writing a cache copy that had nothing to survive to anyway. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TFbiJwd86MZCTLH5kJHZxo * Hoist the async timeout default above the properties Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TFbiJwd86MZCTLH5kJHZxo * Import AsyncTaskRunner in the fake's docblock reference for Pint Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TFbiJwd86MZCTLH5kJHZxo * fix(async): real completion ordering, owner-only spool, handle() guard Three follow-ups from review. drainJumpCompletion() sorted spool files by name, but the names are random UUIDs — so "oldest first" was actually arbitrary order. Sort by filemtime with the name as a tiebreak, since mtime is one-second granular on some filesystems and two tasks can finish inside the same second. Spool files are written 0600 in a 0700 directory (and a directory left world-readable by an earlier run is tightened). The payload goes straight to unserialize() in the runner: a closure is signed with the app key, but a task subclass's constructor arguments are not. forTask() rejects a subclass with no handle() at dispatch time, matching the static-closure guard beside it — otherwise the mistake surfaces as a generic ->failed() from a background thread reading "Call to undefined method". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AMnYeU9ZEGk2XR4FpciYRD * fix(async): don't let the Jump runloop block past a spooled completion On device, AsyncTask.Complete posts a native event that unblocks nativephp_element_wait_event(). Under Jump there is no such wake path — the completion spool is just a directory the polyfill checks on entry. nextEventTimeout() returns -1 (block until the user does something) for any screen without a #[Poll], which is most of them. So the polyfill checked an empty spool, blocked indefinitely, and the completion written a moment later sat there undelivered. From the app it looks like a task that never finishes; dispatch several and they all land at once the moment an unrelated tap happens to wake the loop. While Jump runners are in flight (or a completion is spooled but not yet drained), clamp the wait to 150ms so the loop comes back and looks. Costs nothing when no async task is pending — hasPendingJumpRunners() is false and the timeout is passed through untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AMnYeU9ZEGk2XR4FpciYRD * Caveat the one-outcome guarantee: the event channel can drop frames Shane's finding on #228: nphp_element_post_event() is a single slot, not a queue — a second post landing before PHP drains the first overwrites it. This lane is the first thing posting into it from several OS threads at once, so it's the first to expose it; concurrent completions get dropped, the watchdog's own timeout event included. Every path above the channel does reach an outcome, so the design section stays, but claiming it flatly overstates what ships until the FIFO fix lands in build-scripts (deferred past v4 — needs a PHP rebuild). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TFbiJwd86MZCTLH5kJHZxo * Fix the mount() scoping hole, silent Jump runner deaths, and packaging From @shanerbaner82's review. This is the PHP/packaging slice; the native executor findings are held pending a decision on the hot-reload trade-off. 1a — a task dispatched from mount() scoped to the WRONG screen. mount() runs before the runloop marks itself active, and the router calls it before runLoop() on the hot-swap path, so the canonical 'start loading when the screen opens' dispatch captured the screen being replaced. Its completion was then dropped by the origin check — and so was its timeout failure, by the same check, so nothing could unblock the spinner. Marked active before mount() in run(), and around mount()/onResume() + runLoop() in NativeRouter (markActive/restoreActive are @internal-public for it). 1e — a Jump runner that exited without spooling was dropped along with its deadline, so nothing ever reported on the task. That's the dev loop's likeliest failure: a parse error from a half-saved file, a fatal in bootstrap, an OOM, a SIGKILL. Now synthesized as a failure carrying the exit code and the tail of the runner's stderr, so the cause is visible instead of a generic timeout a minute later. disableOutput() is gone for the same reason — it made getErrorOutput() throw at exactly the moment it was needed. Packaging — composer.json was missing laravel/serializable-closure (used unguarded) and symfony/process. Both only resolved via the host app's laravel/framework. Tests — the refused-dispatch test guarded on function_exists( 'nativephp_call'), which the Jump polyfill defines in every testbench boot, so it had never run; it now drives the refusal through FakeBridge, with the accepted and unanswered cases alongside. The durability test hand-called NativeCallbacks::register(), so flipping durable: in PendingAsyncTask wouldn't have failed it; it now goes through a real dispatch. The fake also round-trips the work envelope through serialize/unserialize, so captured objects are deep copies in tests as they are on a device. Also corrected the serialize-guard comment: PHP 8.4 serializes a captured resource to i:0 without complaint, so that guard never caught the case its own comment named. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TFbiJwd86MZCTLH5kJHZxo * Deep-copy task args in the fake, but not the closure The full envelope round-trip broke 'passes an AsyncTaskException carrying the original message': unserializing a closure re-evaluates its source in the namespace ReflectionClosure reports, which for a closure written in a Pest test file is Pest's compiled namespace rather than the global one it was written in — so an unqualified 'new RuntimeException' resolved to P\Tests\Unit\RuntimeException. That's an artifact of re-evaluating a test-file closure in this process, not something a device does, so reproducing it here fails tests over a problem real dispatches don't have. Round-trip the task-subclass args, which are plain data and where the identity-sharing gap is real, and document the remaining closure divergence rather than papering over it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TFbiJwd86MZCTLH5kJHZxo * Un-stale the event-channel note: format v4 made it a FIFO The single-slot channel this documented was fixed on main (7b38826 plus the extension-side change), so the caveat now describes a bound that no longer exists. Rewritten as history rather than a live limitation. Keeps one residual on the record: post_event returns 1 queued / 0 dropped and drops when PHP has stopped draining. The UI writer discards that by design, but a completion is the case that should check it — and AsyncTask.Complete doesn't yet, which leaves a dropped completion leaning on the watchdog again. Noted as a native follow-up rather than fixed here, since that lane is still uncompiled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TFbiJwd86MZCTLH5kJHZxo * fix(async): allow Symfony Process 8 (#367) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Shane Rosenthal <srosenthal82@gmail.com> Co-authored-by: Kelvin Macharia Ngunyi <ngunyimacharia@gmail.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
3 participants
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.
Summary
symfony/process8.x alongside the existing 6.x and 7.x supportWhy
The current
^6.2|^7.0constraint prevents Composer from resolving #228 in consumers whose policy blocks all matching Symfony 6/7 releases for advisories, even when they already run Symfony Process 8.The package requires PHP
^8.4, which satisfies Symfony Process 8. Direct Process usage on this branch stays within APIs available in Symfony 8.Verification
composer validate --strictSymfony\Component\Processusage against Symfony 8A full upstream
composer update --dry-runremains blocked locally by an unrelated PHP 8.5 / legacy test dependency conflict (orchestra/testbench/ Pest / PHPUnit), not this constraint.