| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Wrap the default platform in NativeScriptPlatform and serve GetForegroundTaskRunner from a per-isolate EventLoop bound to the runtime's CFRunLoop, so v8 foreground tasks (Atomics.waitAsync wakeups, GC tasks, streaming-compilation merge-backs) actually run instead of sitting in never-pumped libplatform queues. Every non-bare entry ends with a microtask checkpoint, fixing the kAuto stall for promises resolved from pure native. Two lanes by ordering contract: the ordered lane rides CFRunLoopPerformBlock tokens (FIFO with foreign performed blocks) and backs the new __ns__queueMacrotask seam; the internal lane rides a version-0 CFRunLoopSource that runs one due entry per pass and re-signals while more are due, hosting v8 tasks, worker->parent messages and the deferred-throw sites. The registry survives isolate-pointer reuse across worker churn via matched erase plus refresh-on-create, and the v8 runner resolves its loop per post. Migrated 1:1 onto the loop: Worker/WorkerWrapper ExecuteOnRunLoop call sites (internal lane), ClassBuilder's cross-thread GcProtect/GcUnprotect and NativeScriptException's deferred @throw blocks (bare entries, own ceremony, outside the loop's exception guard), and both inspector pause loops (PumpMessageLoop -> RunNestableV8Tasks, nestable-only, bounded).
Replace the per-timer CFRunLoopTimers (and the TimerContext retain/release machinery) with the Android runtime's token scheme: scheduled timers keep an exact sub-millisecond sorted list and post one anonymous due token through the EventLoop, whose drain runs the earliest due item across timers and ordered macrotasks - one due-ordered domain, FIFO with performed blocks on the same runloop. clearTimeout/clearInterval leave a tombstone whose own token consumes it as a no-op, so no token gains surplus capacity to run a later-scheduled item ahead of foreign runloop work queued between the two token positions. Intervals realign to startTime + k*frequency and re-arm before the callback runs, preserving the skipped-fire and throwing-callback behavior of the repeating CFRunLoopTimers this replaces.
Atomics.waitAsync notify/timeout/mismatch/promise-chain (the async cases hang without the event loop), the kAuto-stall assertion (microtasks of a natively-resolved promise beat a later macrotask), __ns__queueMacrotask ordering and argument validation, clear-tombstone coverage, and worker specs: message ordering, waitAsync inside a worker's own loop, a reply racing an overdue waitAsync timeout, terminate-with-queued-work, and create/terminate churn exercising the isolate-pointer-reuse registry path.
clearTimeout on a future-due timer now removes its anonymous token from the loop's own bookkeeping and re-arms the wakeup timer, restoring the immediate-cancellation behavior of the CFRunLoopTimerInvalidate path this branch replaced - a cleared long timer no longer wakes the runloop at its original due time just to swallow a tombstone. Tokens are anonymous and counted, so removing one token plus one sorted entry keeps slots 1:1 regardless of which producer's token is physically removed. Tokens already sent out as performed blocks cannot be recalled; those slots keep the tombstone. Also run only ONE matured token per ordered-timer fire, re-arming with the already-past due time so the remainder fires on the next runloop pass: foreign timers and blocks due between two matured tokens now interleave by due time instead of waiting out a batch, matching the one-token-one-message fairness of the Android MessageQueue design.
Two tombstone probes posting a real CFRunLoopPerformBlock between two timer tokens (the cleared timer's token must not run the later timer or a queued macrotask ahead of the native block), and an NSTimer interleave spec: a foreign timer due between two overdue native timers fires between them, not after the batch.
Route every ordered token through the single token CFRunLoopTimer - a due-now token gets a past fire date and fires on the next runloop pass - instead of delivering due-now tokens as performed blocks. This keeps same-instant tokens in fire-date order with foreign NSTimers, exactly like the per-timer CFRunLoopTimers the event loop replaced, and each fire yields a full runloop pass, so a self-rescheduling setTimeout(0) chain cannot monopolize block servicing. The yield spec measures this through the runtime's own before-waiting observer (the rejection drain): a rejection injected mid-chain must be reported while the chain is still running, alongside a foreign NSTimer and a GCD main-queue block. CFRunLoopObserver handlers created from JS never fire, so the drain is the only trustworthy probe.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: cdcfebbb-6b69-4788-a8eb-782478dc6f63 📥 CommitsReviewing files that changed from the base of the PR and between dc2081a and 7c72b7f. 📒 Files selected for processing (9)
📝 Walkthrough WalkthroughThe runtime adds a CFRunLoop-based EventLoop and connects it to V8 foreground tasks, timers, workers, exceptions, inspectors, top-level await, and ordered macrotasks. The previous SetTimeout implementation is removed. New tests cover scheduling and lifecycle behavior. ChangesRuntime event loop integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Runtime
participant NativeScriptPlatform
participant EventLoop
participant V8
Runtime->>NativeScriptPlatform: Register isolate event loop
V8->>NativeScriptPlatform: Request foreground task runner
NativeScriptPlatform->>EventLoop: Post V8 task
EventLoop->>V8: Run task and microtasks
Possibly related issues
Possibly related PRs
Suggested reviewers: nathanwalker Poem 🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
Never initialized (its Init call has been commented out since the timers rework) and hardcoded to the main queue; the event loop's ordered lane is the replacement seam.
The module evaluation spin only performed microtask checkpoints, so an await whose resolution arrives as a v8 foreground task could never settle within the wait. Drain the event loop's nestable tasks each iteration - JS frames are on the stack, so like the inspector pause loops non-nestable tasks (e.g. Atomics.waitAsync wakeups, which v8 posts as non-nestable) stay queued and run from their own wakeups after the turn; the specs pin both behaviors.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)NativeScript/runtime/ClassBuilder.mm (1)321-342: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Inconsistent null handling for GetEventLoop() across migrated call sites. WorkerWrapper::PostToRuntimeLoop (NativeScript/runtime/WorkerWrapper.mm:26-29) and ScheduleDeferredThrow (NativeScript/runtime/NativeScriptException.mm:230-233) both treat a null loop as reachable, but these three migrated sites dereference the result directly. Apply the same guard at each site.
🤖 Prompt for AI Agents
- NativeScript/runtime/ClassBuilder.mm#L321-L342: check Runtime::GetRuntime(isolate) before RuntimeLoop(), and check GetEventLoop() before PostInternalBare. Apply the same change to the swizzled release at lines 379-384.
- NativeScript/runtime/Worker.mm#L314-L314: store runtime->GetEventLoop() in a local, return early when it is null, then call PostInternal.
- NativeScript/runtime/Timers.cpp#L231-L237: check Runtime::GetRuntime(isolate) and the returned loop before calling SetTimerSource, and keep postToken consistent with that check.
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/ClassBuilder.mm` around lines 321 - 342, Add consistent null guards for runtime and event-loop access: in NativeScript/runtime/ClassBuilder.mm#L321-L342 and the swizzled release at lines 379-384, validate Runtime::GetRuntime(isolate) before RuntimeLoop() and GetEventLoop() before PostInternalBare; in NativeScript/runtime/Worker.mm#L314-L314, store GetEventLoop() locally, return when null, then call PostInternal; in NativeScript/runtime/Timers.cpp#L231-L237, validate the runtime and loop before SetTimerSource and keep postToken aligned with that guarded path.
NativeScript/runtime/ClassBuilder.mm (1)🤖 Prompt for all review comments with AI agents379-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Reuse gcUnprotect in the same-thread branch.
Lines 386-401 repeat the body of the gcUnprotect lambda defined at line 362. The retain path already calls its lambda directly in the same-thread branch. Call gcUnprotect() here so the two paths cannot diverge.
♻️ Proposed refactor🤖 Prompt for AI Agents} else { - auto innerCache = isolateWrapper.GetCache(); - auto it = innerCache->Instances.find(self); - if (it != innerCache->Instances.end()) { - v8::Locker locker(isolate); - Isolate::Scope isolate_scope(isolate); - HandleScope handle_scope(isolate); - if (it->second != nullptr) { - Local<Value> value = it->second->Get(isolate); - BaseDataWrapper* wrapper = tns::GetValue(isolate, value); - if (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject) { - ObjCDataWrapper* objcWrapper = static_cast<ObjCDataWrapper*>(wrapper); - objcWrapper->GcUnprotect(); - } - } - } + gcUnprotect(); }Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/ClassBuilder.mm` around lines 379 - 401, In the same-thread branch of the runtime callback, replace the duplicated cache lookup, V8 scope setup, and ObjC wrapper unprotect logic with a direct call to the existing gcUnprotect lambda. Preserve the surrounding runtime-loop branching and keep the existing bare PostInternalBare(gcUnprotect) path unchanged.
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Inline comments: In `@NativeScript/inspector/JsV8InspectorClient.mm`: - Around line 440-442: Update JsV8InspectorClient::runMessageLoopOnPause() to exit its main pause-loop iteration before Runtime::~Runtime() removes the isolate’s event-loop entry; coordinate shutdownRuntime() with that loop so no iteration calls GetEventLoop() after runtime_ is destroyed, rather than relying on a null guard. In `@NativeScript/runtime/EventLoop.h`: - Around line 94-97: Update the ordered-lane documentation at NativeScript/runtime/EventLoop.h:94-97 to describe delivery through the single due-ordered timer and its FIFO contract, not CFRunLoopPerformBlock; at NativeScript/runtime/EventLoop.h:108-116 remove the performed-block case and retain only the matured case. In NativeScript/runtime/Runtime.mm:589-590, describe the ordered-lane timer behavior and state that FIFO is maintained against JS timers through the shared due-ordered domain. In `@NativeScript/runtime/Timers.cpp`: - Around line 231-237: Guard the event loop before calling SetTimerSource in Timers::Init, and add the same null handling to postToken where eventLoop_ is dereferenced. Handle a null Runtime::GetRuntime(isolate) or event loop with the existing predictable failure behavior, while preserving TimerState cleanup semantics. - Around line 111-115: Update the timer bookkeeping shared by postToken and removeTask so cancellation tracks the effective ordered-token key actually posted: use now when dueTime is overdue, otherwise dueTime, and pass that same key to TryCancelOrderedToken. Preserve one-to-one token accounting for equal-due macrotasks. In `@NativeScript/runtime/WorkerWrapper.mm`: - Around line 34-41: Update the PostInternal completion flow in WorkerWrapper to ensure every successfully posted task signals or otherwise releases the waiting worker when EventLoop::Shutdown() drops the entry; alternatively replace the unbounded dispatch_semaphore_wait with an appropriate bounded wait and handle timeout. Preserve normal fn execution and completion signaling for tasks that run. In `@TestRunner/app/tests/EventLoopTests.js`: - Around line 378-390: Update the worker onmessage handler in the “survives terminating a worker with queued loop work” test to use a one-shot guard, ensuring the message-posting, terminate call, and delayed done callback execute only on the first delivery; ignore subsequent messages. --- Outside diff comments: In `@NativeScript/runtime/ClassBuilder.mm`: - Around line 321-342: Add consistent null guards for runtime and event-loop access: in NativeScript/runtime/ClassBuilder.mm#L321-L342 and the swizzled release at lines 379-384, validate Runtime::GetRuntime(isolate) before RuntimeLoop() and GetEventLoop() before PostInternalBare; in NativeScript/runtime/Worker.mm#L314-L314, store GetEventLoop() locally, return when null, then call PostInternal; in NativeScript/runtime/Timers.cpp#L231-L237, validate the runtime and loop before SetTimerSource and keep postToken aligned with that guarded path. --- Nitpick comments: In `@NativeScript/runtime/ClassBuilder.mm`: - Around line 379-401: In the same-thread branch of the runtime callback, replace the duplicated cache lookup, V8 scope setup, and ObjC wrapper unprotect logic with a direct call to the existing gcUnprotect lambda. Preserve the surrounding runtime-loop branching and keep the existing bare PostInternalBare(gcUnprotect) path unchanged.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 213b5345-cf4e-4f1a-baf4-dc5911f8c796
📥 CommitsReviewing files that changed from the base of the PR and between 32ed9e5 and dc2081a.
📒 Files selected for processing (25)
Sorry, something went wrong.
- Inspector pause loops resolve the loop via LookupEventLoop with a null guard: GetEventLoop creates on miss, so a stale pause iteration could mint a registry entry for an isolate whose runtime is gone. - PostOrderedToken returns the key it actually recorded (an overdue due time is clamped to the loop's now); Timers stores it per cycle as postedTokenTime_ and cancels by it, so clearing an overdue timer recalls the token instead of leaving a tombstone plus a wakeup. - The sync PostToRuntimeLoop wait is released even when Shutdown drops the entry unrun: a shared_ptr deleter signals the semaphore when the posted closure is destroyed, whichever way that happens. - Timers::Init and postToken guard a null runtime/event loop, degrading to inert timers instead of crashing. - Stale performed-block wording in the ordered-lane docs replaced with the timer-phase contract. - One-shot guard in the terminate-with-queued-work spec: late echoes must not re-drive the handler and double-invoke done().
PostOrderedToken returns the recorded key as the producer's recall handle; the bind-time flush re-clamped buffered keys against the bind time, so a pre-bind poster whose due time had passed could never recall its token. Insert buffered keys verbatim - a past key simply fires on the next runloop pass. Also note the reused-isolate-address window (tasks v8 posts during Isolate::New land in the stale stopped loop until the refresh right after) and finish the stale performed-block wording in Timers.
| Back | FazBrowse Home | New Git URL |
Problem
Nothing pumps the V8 platform's foreground task queues. v8::platform::PumpMessageLoop only ran inside the two debugger pause loops (JsV8InspectorClient/WorkerInspectorClient). In normal execution, every task V8 posts to its foreground runner just sat there:
Separately, "get work onto the JS thread" had grown four bespoke mechanisms: tns::ExecuteOnRunLoop (CFRunLoopPerformBlock + wakeup), two raw perform-block bypasses in NativeScriptException.mm, Timers.cpp's per-timer CFRunLoopTimers, and ConcurrentQueue's runloop source for inbound worker messages. No shared ordering domain, and no per-entry microtask handling: with MicrotasksPolicy::kAuto, work that resolves a promise without entering JS (exactly the Atomics.waitAsync shape) stalls until unrelated JS runs.
Change: a per-runtime EventLoop with two lanes
Port of the Android runtime's event loop (NativeScript/android#2003), same API and contracts, CFRunLoop-native implementation. Each Runtime owns an EventLoop, bound to its home thread in CreateIsolate, accepting posts from any thread and buffering until bound. Work is routed by ordering contract:
Ordered lane — work whose ordering is observable against app-level runloop work. Every post is an anonymous "task due" token on ONE CFRunLoopTimer armed at the earliest pending due time; a due-now token gets a past fire date and fires on the next runloop pass, one token per fire. This keeps tokens in fire-date order with foreign NSTimers (exactly like the per-timer CFRunLoopTimers this replaces) and guarantees a full runloop pass between consecutive tokens — measured via the runtime's own before-waiting rejection-drain observer, a rejection injected mid-chain drains between steps (step 5 of 50), identical to the NSTimer-chain reference, so self-rescheduling setTimeout(0) chains starve neither rendering commits nor the autorelease pool nor the drain. Each token runs the earliest due item across the ordered entries and the OrderedTaskSource (Timers) — one due-ordered domain. First producer: __ns__queueMacrotask(cb), the seam future spec'd macrotasks (e.g. performance-observer callbacks) will use.
Internal lane — work in its own ordering domain: v8 platform foreground tasks (Atomics.waitAsync wakeups, GC tasks), worker→parent messages, exception/rejection deliveries. Rides a version-0 CFRunLoopSource plus one CFRunLoopTimer for delayed work. The source's perform callback runs exactly one due entry and re-signals itself while more are due, so bursts interleave with other runloop work instead of draining in one go — the CFRunLoop analogue of Android's one-eventfd-unit-per-poll. (Because the drain is queue-state-driven rather than unit-counted, Android's eventfd unit-accounting hazard has no iOS equivalent; the regression test is ported anyway.)
NativeScriptPlatform wraps the default platform (workers/jobs/time/tracing delegate to libplatform) and serves GetForegroundTaskRunner(isolate) from the isolate's EventLoop. The loop starts unbound and buffers (v8 requests the runner during Isolate::New); binding flushes. Each executed non-bare entry runs under Locker + isolate/handle/context scopes and ends with a microtask checkpoint — the kAuto-stall fix. Shutdown drops queued work and late posts.
Lane mapping rationale (vs Android's Java Handler + eventfd): iOS has no single app-level message queue — CFRunLoop services performed blocks and timers in different phases, so the ordered lane cannot be FIFO with both. It rides the timer phase: that is where the pre-existing per-timer CFRunLoopTimers lived, so cross-ordering against app NSTimers is preserved (fire-date order), and a due timer still yields a full runloop pass between fires. (A performed-block carrier was prototyped and rejected: blocks would be FIFO with ExecuteOnRunLoop-style posts, but same-instant ordering against NSTimers would flip phase — timers were never ordered against blocks on main either, so the timer phase is the least-surprise choice.) The internal lane is a version-0 CFRunLoopSource: the natural "own ordering domain" primitive, serviced with no ordering contract against blocks or timers, exactly the lane's contract. Both lanes are real mechanisms, not a collapsed queue, so subsystem code ports between the platforms without rethinking which lane it belongs to.
Timers merged into the ordered lane (with tombstones)
The per-timer CFRunLoopTimers (and the retain/release TimerContext machinery) are gone; timers post anonymous tokens through the EventLoop, and the token drain runs the earliest due item across timers and ordered macrotasks under one Locker acquisition (Timers implements OrderedTaskSource::RunIfEarliest). clearTimeout recalls the timer's token outright when it is still future-due (removing the armed wakeup — the same immediate cancellation CFRunLoopTimerInvalidate gave), and leaves a tombstone when the token has already matured (or was posted due-now), so no token gains surplus capacity to run a later-scheduled item ahead of foreign runloop work queued between the two token positions. The token wakeup timer runs one matured token per fire (re-armed with the past due time, firing again next pass), so foreign timers and blocks due between two matured tokens interleave by due time — the one-token-one-message fairness of the Android MessageQueue design. Interval catch-up semantics (NextTime: realign to startTime + k*frequency, matching the skipped-fire behavior of repeating CFRunLoopTimers) are preserved; intervals re-arm before the callback runs so a throwing callback can't kill them.
Consolidated onto the loop
Teardown / isolate-pointer reuse
~Runtime shuts the loop down inside its Locker block before any handle disposal; posts from other threads start dropping there. The platform registry entry is removed by a matched erase (IsolateDisposed(isolate, loop)) so a worker isolate that reuses a disposed isolate's address can't be evicted by the previous tenant's late destructor; CreateIsolate additionally refreshes a stopped loop found under its key, and the v8 task runner resolves the loop through the registry on every post so a refresh redirects already-handed-out runners.
Post-review hardening
CodeRabbit + an independent deep review; fixes applied: inspector pause loops resolve the loop via a non-creating lookup (a stale pause iteration could otherwise mint a registry entry for a disposed isolate); PostOrderedToken returns the key it actually recorded and clearTimeout recalls by it (an overdue timer's clear otherwise degraded to tombstone + spurious wakeup); the sync worker-post wait is released via an RAII completion even when Shutdown drops the entry unrun; the bind-time flush replays buffered tokens under their original keys so the recall contract survives pre-bind posts; null guards in Timers::Init/postToken.
Known/accepted notes from review: v8 tasks posted during Isolate::New for a worker that reuses a disposed isolate's address land in the stale stopped loop and are dropped until the immediate post-creation refresh (unfixable without a v8 hook; same shape as android#2003, and at-worst-parity with main where no foreground task ever ran). The tombstone branch is only reachable on an exact due-time tie (sub-nanosecond double), so the tombstone specs exercise token recall; the machinery is verified by review. Same open question as android#2003: microtask checkpoints now run after nestable tasks during debugger pauses (Blink parity).
Not in this PR
Tests
EventLoopTests.js (+ echo/waitAsync workers): Atomics.waitAsync notify/timeout/mismatch/promise-chain — the async cases hang without this change; natively-resolved promise microtasks run before a later macrotask (the kAuto-stall assertion); __ns__queueMacrotask async delivery, runs-after-microtasks, FIFO among itself and with __ns__setTimeout(0), TypeError on non-function; clear-tombstone specs (clear + re-schedule doesn't reorder, overdue clear doesn't fire a later timer early, interval survives unrelated clear); worker specs (message ordering, waitAsync inside a worker's own loop, reply racing an overdue waitAsync timeout, terminate-with-queued-work, 8× churn exercising the registry-refresh path).
Also covered: tombstone probes posting a real CFRunLoopPerformBlock between two timer tokens, an NSTimer interleave spec (a foreign timer due between two overdue native timers fires between them), and a yield spec driving a 50-step immediate-timer chain while asserting a foreign NSTimer, a main-queue block and the before-waiting rejection drain all land mid-chain.
Full suite: 1077 specs green before, 1101 green after (same simulator).