| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting. Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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: b8709840-58cc-4d09-acfb-357c965daab7 📥 CommitsReviewing files that changed from the base of the PR and between c2be640 and d89d08e. 📒 Files selected for processing (5)
📝 Walkthrough WalkthroughThe runtime replaces legacy looper and timer infrastructure with per-isolate EventLoop scheduling. NativeScriptPlatform adapts V8 tasks. Timers, callbacks, promises, workers, inspector pauses, and tests now use the new event-loop paths. ChangesUnified event-loop refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: ⚪ Minimal · up to d89d0 The scheduler and timer changes include fixes for identified edge cases and regression coverage; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant JavaScript
participant CallbackHandlers
participant NativeScriptPlatform
participant EventLoop
participant EventLoopHandler
participant Timers
JavaScript->>CallbackHandlers: Queue macrotask
CallbackHandlers->>NativeScriptPlatform: Resolve isolate runner
NativeScriptPlatform->>EventLoop: Post ordered task
EventLoop->>EventLoopHandler: Post task token
EventLoopHandler->>EventLoop: Invoke nativeRunTask
EventLoop->>Timers: Arbitrate earliest timer
EventLoop->>JavaScript: Execute task and microtasks
Possibly related PRs
Suggested reviewers: nathanwalker Poem 🚥 Pre-merge checks | ✅ 4 | ❌ 1 ❌ Failed checks (1 warning)
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.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)test-app/runtime/src/main/cpp/NativeScriptPlatform.h (1)🤖 Prompt for all review comments with AI agents120-124: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Consider guarding the static JNI cache with std::once_flag.
Each ForegroundTaskRunner owns its own mutex_, so the lock in BindToCurrentThread does not serialize writes to these process-wide statics across runners. The current safety argument depends on the main runtime always binding before any worker. std::call_once would remove that dependency and would survive a future change to worker startup order.
♻️ Suggested guard// process-wide JNI cache, written once under the first bind's lock + static std::once_flag EVENT_LOOP_HANDLER_INIT; static jclass EVENT_LOOP_HANDLER_CLASS;Then wrap the lookup block in BindToCurrentThread with std::call_once(EVENT_LOOP_HANDLER_INIT, ...).
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/cpp/NativeScriptPlatform.h` around lines 120 - 124, Guard initialization of the process-wide JNI cache used by ForegroundTaskRunner::BindToCurrentThread with a shared std::once_flag, such as EVENT_LOOP_HANDLER_INIT. Move the class and method lookups into std::call_once so initialization is serialized across all runners, while preserving the existing cached symbols and subsequent use.
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 `@test-app/app/src/main/assets/app/tests/testEventLoop.js`: - Around line 52-59: Add a rejection handler to the Atomics.waitAsync promise chain so rejected promises and assertion errors call done.fail with the captured error, matching the handling in the other asynchronous tests and preventing unhandled rejections. In `@test-app/runtime/src/main/cpp/NativeScriptPlatform.cpp`: - Around line 82-99: Synchronize handler lifetime with posting: in test-app/runtime/src/main/cpp/NativeScriptPlatform.cpp lines 82-99, take mutex_ in ForegroundTaskRunner::~ForegroundTaskRunner before accessing handler_, call DeleteGlobalRef while holding it, and clear handler_ under the same lock; in lines 106-139, keep mutex_ held through PostToken in both PostImmediate and PostDelayed so posting cannot overlap destruction. - Around line 215-233: Update ForegroundTaskRunner::RunNestableTasks to capture the entry-time due-task boundary before draining, then process only tasks that were due at that point. Ensure tasks reposted during task->Run() are not consumed in the same invocation, allowing the inspector pause loop to return and read the next CDP message. In `@test-app/runtime/src/main/cpp/Runtime.cpp`: - Around line 299-301: Guard the NativeScriptPlatform::Instance() and m_isolate values before calling IsolateDisposed in ~Runtime, so destruction before PrepareV8Runtime initialization is safe. Inspect DestroyRuntime and tns::disposeIsolate to verify disposal completes synchronously and that ~Runtime runs only afterward; if disposal is deferred, adjust the ordering so IsolateDisposed executes after v8::Isolate::Dispose completes and before the isolate mapping is forgotten. In `@test-app/runtime/src/main/java/com/tns/EventLoopHandler.java`: - Around line 26-29: Update EventLoopHandler’s constructor to validate Looper.myLooper() before passing it to Handler, and fail with a clear diagnostic when no Looper is prepared. In ForegroundTaskRunner::BindToCurrentThread, check env.ExceptionCheck() immediately after env.NewObject and stop the binding flow before creating a global reference or storing handler_ when construction fails. --- Nitpick comments: In `@test-app/runtime/src/main/cpp/NativeScriptPlatform.h`: - Around line 120-124: Guard initialization of the process-wide JNI cache used by ForegroundTaskRunner::BindToCurrentThread with a shared std::once_flag, such as EVENT_LOOP_HANDLER_INIT. Move the class and method lookups into std::call_once so initialization is serialized across all runners, while preserving the existing cached symbols and subsequent use.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b8fba76-b1fd-4594-9322-720eefa43cf1
📥 CommitsReviewing files that changed from the base of the PR and between f284059 and cb526bc.
📒 Files selected for processing (13)
Sorry, something went wrong.
|
Addressed the CodeRabbit findings; note the runner has since been restructured into a two-lane EventLoop (see updated PR description), so some fixes landed in EventLoop.cpp rather than the file the comment anchored to:
|
Sorry, something went wrong.
Scheduler design review (independent deep review) — outcomeCurrent implementation: two must-fix defects found and fixed in the latest push:
Multithreaded JS: no new exposure. No v8::Unlocker exists anywhere, so the home-thread checkpoint can only observe completed background turns; kAuto already allowed any-thread drains. Home-thread Locker stalls behind long background JS turns are pre-existing (Timers has the identical shape) and granularity improved (one entry per poll vs captive batches). __ns__queueMacrotask from background-thread JS is safe and semantically sane. Merge proposal (Timers + ordered lane into one token stream): endorsed, with one addition. Adversarial analysis confirmed a leftover clearTimeout token could run a later-posted item ahead of foreign Java messages — and found shipped Timers already exhibits exactly this deviation under congestion. Adopting tombstones on clear (cancelled entries no-op in their slot instead of being deleted) keeps tokens and entries 1:1 in due order, making the merged scheme strictly tighter than either predecessor. Migration guidance: merge only the outer token stream + due-selection; keep FireTimer's internals (interval catch-up, nesting clamp, TryCatch discipline) untouched. Agreed sequencing: (a) this PR with the fixes above → (b) Timers/ordered-lane merge with tombstones → (c) __runOnMainThread promotion into the internal lane with an own-isolate entry flavor (routing it through the ordered lane would nest main-isolate and worker-isolate Lockers and can deadlock against multithreaded-JS entry paths) → (d) kExplicit microtask work (by then the kAuto-reliant sites are down to the JNI trampolines + ModuleInternal) → (e) budgeted internal-lane batch drain, profiling-gated. Open questions for maintainers:
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)test-app/runtime/src/main/cpp/CallbackHandlers.cpp (1)🤖 Prompt for all review comments with AI agentstest-app/runtime/src/main/cpp/Timers.cpp (1)809-819: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Catch std::exception by const reference.
Line 811 catches by value. The copy slices any derived exception, so e.what() at Line 813 reports the base std::exception text instead of the real message. Catch by const std::exception&.
♻️ Proposed refactor🤖 Prompt for AI Agents- } catch (std::exception e) { + } catch (const std::exception& e) { stringstream ss; ss << "Error: c++ exception: " << e.what() << endl;Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/cpp/CallbackHandlers.cpp` around lines 809 - 819, Update the std::exception handler in CallbackHandlers.cpp to catch the exception as const std::exception& instead of by value, while preserving the existing e.what() logging and rethrow flow.test-app/runtime/src/main/cpp/Timers.h (1)244-248: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Use the now parameter instead of recomputing the time.
RunIfEarliest receives now, then compares against a fresh now_ms() at Line 245. The otherDue value was computed by the event loop against the passed now. Two different time bases make the "earliest across both domains" decision inconsistent: a timer whose dueTime falls between now and now_ms() becomes eligible while the loop treated no ordered entry as due. Use the parameter for one consistent basis.
♻️ Proposed refactor🤖 Prompt for AI Agentsauto ref = sortedTimers_.front(); - if (ref.dueTime > now_ms() || (otherDue >= 0 && ref.dueTime > otherDue)) { + if (ref.dueTime > now || (otherDue >= 0 && ref.dueTime > otherDue)) { // not due, or the loop's own entry is earlier - not this source's slot return false; }Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/cpp/Timers.cpp` around lines 244 - 248, Update RunIfEarliest to compare ref.dueTime against its now parameter instead of calling now_ms(), while preserving the existing otherDue comparison and return behavior.139-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Align the thread-safety comment with the actual locking model.
This comment states that sortedTimers_ is "Only ever touched on the isolate's home thread, no mutex". Timers::RunIfEarliest in Timers.cpp states the opposite: sortedTimers_ is mutated through setTimeout from background threads under multithreaded JS, and the isolate Locker is the guard. OrderedTaskSource in EventLoop.h documents the same Locker-based contract. Update this comment so future readers do not remove the Locker acquisition.
📝 Proposed comment fix🤖 Prompt for AI Agents// scheduled timers (and tombstones) sorted by exact (sub-millisecond) - // dueTime, stable for equal dueTimes. Only ever touched on the - // isolate's home thread, no mutex. The Java message queue is - // millisecond-quantized, so this preserves the relative order of JS - // timers; each anonymous EventLoop token consumes the front slot. + // dueTime, stable for equal dueTimes. Guarded by the isolate Locker, + // not a mutex: background threads mutate it through setTimeout under + // multithreaded JS. The Java message queue is millisecond-quantized, + // so this preserves the relative order of JS timers; each anonymous + // EventLoop token consumes the front slot.Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/cpp/Timers.h` around lines 139 - 144, Update the comment above sortedTimers_ to document that access is synchronized by the isolate Locker, including mutations from background threads via setTimeout; remove the inaccurate home-thread-only and no-mutex claims, consistent with Timers::RunIfEarliest and OrderedTaskSource.
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 `@test-app/app/src/main/assets/app/tests/testEventLoop.js`: - Around line 59-61: Replace the unsupported done.fail call in the promise rejection handler of testEventLoop with Jasmine 2.0.1’s explicit failure assertion, then call done() afterward so the handler always completes and reports the original error. In `@test-app/runtime/src/main/cpp/CallbackHandlers.cpp`: - Around line 687-701: Resolve and validate Runtime::GetMainEventLoop() before the cache insertion block in the callback registration flow. Update the surrounding logic so a null mainLoop returns without calling cache_.try_emplace, while valid loops retain the existing insertion, duplicate assertion, and PostInternalBare behavior. - Around line 704-717: Update CallbackHandlers::RunMainThreadEntry so the cached isolate remains alive from cache lookup through v8::Locker acquisition, rather than copying an unprotected raw pointer after releasing cacheMutex_. Use the existing ownership or liveness mechanism for the cache entry, and ensure teardown cannot dispose the isolate until the lock-acquisition phase completes. --- Nitpick comments: In `@test-app/runtime/src/main/cpp/CallbackHandlers.cpp`: - Around line 809-819: Update the std::exception handler in CallbackHandlers.cpp to catch the exception as const std::exception& instead of by value, while preserving the existing e.what() logging and rethrow flow. In `@test-app/runtime/src/main/cpp/Timers.cpp`: - Around line 244-248: Update RunIfEarliest to compare ref.dueTime against its now parameter instead of calling now_ms(), while preserving the existing otherDue comparison and return behavior. In `@test-app/runtime/src/main/cpp/Timers.h`: - Around line 139-144: Update the comment above sortedTimers_ to document that access is synchronized by the isolate Locker, including mutations from background threads via setTimeout; remove the inaccurate home-thread-only and no-mutex claims, consistent with Timers::RunIfEarliest and OrderedTaskSource.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 176a9121-a7f6-46ca-bc75-1b507efbd79e
📥 CommitsReviewing files that changed from the base of the PR and between cb526bc and 33d7547.
📒 Files selected for processing (23)
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agentsVerify 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 `@test-app/app/src/main/assets/app/tests/testEventLoop.js`: - Around line 177-196: Update the background-thread clear flow in the test iteration to use an AtomicBoolean or equivalent completion signal set after __ns__clearTimeout(t1) runs. Before completing the iteration with done(), assert that the signal confirms the clear operation executed, while preserving the existing order assertions and retry behavior. In `@test-app/runtime/src/main/cpp/EventLoop.cpp`: - Around line 284-296: Update the token-posting flow around claimCells_ and EVENT_LOOP_HANDLER_POST_TOKEN so a failed env.CallVoidMethod releases the claimed cell back to 0 when no token reaches the queue. Preserve the existing dispatch-gate cleanup for successfully queued active tokens, and ensure the failure path is triggered only when the JNI post throws or otherwise does not complete. - Around line 102-116: Update the EventLoop native binding around EventLoop::ClaimTokenCritical to track whether critical registration succeeds and whether the runtime supports the critical JNI ABI (API 26+); handle RegisterNatives failure without leaving a pending exception. Gate claim-token behavior on that capability, keep nativeClaimToken provided only through RegisterNatives, and make PostTimerToken emit plain tokens whenever the critical binding is unavailable so EventLoopHandler.handleMessage uses the compatible legacy path. In `@test-app/runtime/src/main/cpp/Timers.cpp`: - Around line 263-270: Ensure failed token posts roll back committed state: in test-app/runtime/src/main/cpp/Timers.cpp lines 263-270, update addTask around postTimer so exceptions remove the timerMap_ entry and sortedTimers_ slot before propagating to the existing catch; in test-app/runtime/src/main/cpp/EventLoop.cpp lines 284-296, update the env.CallVoidMethod exception path to store 0 in the claim cell before propagating the exception.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6de1b1b0-c38a-4bbb-9d5f-fff507bf1c32
📥 CommitsReviewing files that changed from the base of the PR and between 33d7547 and 375eecd.
📒 Files selected for processing (6)
Sorry, something went wrong.
V8 platform foreground tasks (async WASM compilation callbacks, Atomics.waitAsync wakeups, GC finalization tasks) sat in the default platform's internal queues, which nothing pumped outside the WASM-scoped MessageLoopTimer (an ALooper fd fed by a detached 100ms-polling thread) and the inspector pause loops. Atomics.waitAsync promises never resolved at all. Wrap the default platform in NativeScriptPlatform: worker-thread scheduling, jobs, time and tracing still delegate to libplatform, but GetForegroundTaskRunner serves a per-isolate ForegroundTaskRunner that delivers tasks through a dedicated com.tns.EventLoopHandler bound to the runtime thread's Looper - the same anonymous-token scheme Timers use, so platform tasks are strictly FIFO-ordered with Handler.post runnables and JS timers on the same looper: - each posted task enqueues into a native queue (immediate deque plus a due-time-sorted delayed map) and posts one "task due" token; a token runs the earliest due task, then performs a microtask checkpoint, since a task may resolve promises without entering JS (e.g. Atomics.waitAsync), which kAuto's depth-0 drain never sees - delayed tasks ride sendMessageAtTime at ceil(dueTime), so a token never arrives before its due time - v8 requests the runner during Isolate::New, before the home thread is known, so the runner starts unbound and buffers; PrepareV8Runtime binds it to the thread's Looper and flushes one token per buffered task; posts are accepted from any thread - inspector pause loops can't receive tokens (the Java looper isn't spinning), so they drain nestable tasks directly; non-nestable tasks keep their queued tokens until the pause unwinds, and leftover tokens no-op like cleared-timer tokens - the runner shuts down in DestroyRuntime and is unregistered after isolate disposal, so workers can churn without leaking map entries MessageLoopTimer, its polling thread and the WebAssembly method proxies in message-loop-timer.js are removed: async WASM promises now resolve promptly through the runner with no start/stop windows. The runner is also the seam for future macrotask dispatch (e.g. performance API observer callbacks). Microtask policy is deliberately untouched. Adds Atomics.waitAsync regression tests (notify, timeout, sync mismatch, promise-chain ordering); the async cases hang without this change.
…d lane) Restructure the foreground task runner into a per-runtime EventLoop, the Android analogue of the iOS runtime's ExecuteOnRunLoop seam, routing work by ordering contract: - ordered lane: work whose ordering is observable against app-level Java messages rides the Java MessageQueue via EventLoopHandler tokens, strictly FIFO with Handler.post and JS timers. First producer: __ns__queueMacrotask(cb), the seam future spec'd macrotasks (performance observers etc.) will use. - internal lane: work in its own ordering domain - v8 platform foreground tasks, worker->parent messages, unhandled-rejection drains - rides an EFD_SEMAPHORE eventfd plus a timerfd for delayed tasks on the thread's ALooper. No JNI on the post path, so v8's non-JVM worker threads post without attaching to the JVM. One eventfd unit runs one entry per looper callback, keeping bursts fair with Java messages. LooperTasks is consolidated into the internal lane (worker messaging and exception-drain call sites ported 1:1, keeping the weak_ptr child semantics and drop-after-shutdown behavior). Timers stays separate: it is the ordered lane specialized with sub-millisecond ordering machinery. Also addresses review findings: ordered-lane token posts and destructor now synchronize on the loop mutex; the inspector pause drain is bounded to the entries present at call time so a self-reposting task cannot wedge the CDP read; ~Runtime guards the platform instance and isolate against early construction failure; EventLoopHandler fails loudly when constructed on a thread with no prepared Looper; the async waitAsync test chain got its missing rejection handler. Adds ordered-lane tests: async delivery, runs-after-microtasks, FIFO interleaving with setTimeout(0), TypeError on non-function.
…ign review Two defects found by deep review of the scheduler: - internal-lane unit starvation: an eventfd unit written for an immediate entry could be consumed by a due-but-unsignaled delayed entry (whose own timerfd unit hadn't been issued yet); the timer fire then found nothing due and issued nothing, leaving the lane permanently off-by-one - the newest entry always waited for a future post. The unit-consuming path now skips unsignaled delayed entries; nested (unit-free) drains and the ordered lane are unaffected, since ordered entries carry their token from post time. - stale loop registry across isolate-pointer reuse: the registry erase ran in ~Runtime, several JNI calls after Isolate::Dispose freed the address. A concurrently created worker isolate could reuse the pointer, inherit the dead runtime's stopped loop (silently dropping all its work), and then lose its own entry to the late destructor. The erase now happens immediately after Dispose and only while the entry still maps to the disposing runtime's loop; PrepareV8Runtime 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 also redirects runners v8 already holds. Also from review: the inspector-pause drain no longer lets C++ exceptions unwind through v8 inspector frames, and fd callbacks ignore spurious wakeups instead of consuming an entry. Tests: worker reply racing an overdue Atomics.waitAsync timeout (unit accounting), worker churn smoke, and __ns__queueMacrotask posted from a background JS thread landing on the main thread (multithreaded JS).
…inThread through the internal lane Timers merge (with tombstones): - Timers no longer owns a Java Handler: each scheduled timer posts one anonymous token through the EventLoop's ordered lane, and the token drain runs the earliest due item across timers and ordered macrotasks - one due-ordered domain, still strictly FIFO with Handler.post on the same looper. Token 'when' computation is unchanged, so the quiescent setTimeout-vs-Handler.post contract is preserved exactly. - clearTimeout/clearInterval tombstone the sorted entry instead of erasing it: the cleared timer's already-queued token consumes its own slot as a no-op, so no token gains surplus capacity to run a later-scheduled item (timer or macrotask) ahead of foreign Java messages queued between the two token positions. This also fixes the pre-existing congestion deviation where a leftover token could fire a later timer early. - FireTimer's internals (sub-ms sorted list, chromium-style interval catch-up, nesting clamp, TryCatch discipline) are untouched; the check-and-run happens in one OrderedTaskSource::RunIfEarliest call under a single Locker acquisition, because background threads mutate the timer bookkeeping through setTimeout under multithreaded JS. - TimerHandler.java is deleted. __runOnMainThread promotion: - The 2MB main-looper pipe and RunOnMainThreadFdCallback are replaced by bare internal-lane entries on the main runtime's EventLoop. Bare entries skip the loop's Locker/checkpoint: the closure locks the CALLER's isolate (a worker's, under multithreaded JS), and taking the main isolate's Locker first would nest Lockers across isolates and can deadlock against worker->main JNI entry paths. Delivery stays one-per-poll, matching the old fd callback. - The callback cache is now mutex-guarded: it was written from arbitrary threads under different isolates' Lockers, which provide no mutual exclusion; RemoveIsolateEntries also no longer erases while range-iterating. - Uncaught exceptions in the callbacks now surface as pending Java exceptions via the loop's guard instead of unwinding C++ through the ALooper callback frame. Tests: tombstone ordering specs (cleared timer's token vs java posts, for both a later timer and a queued macrotask), against the native __ns__ timers - the test app's global setTimeout is an old Handler-based polyfill with colliding ids, not the runtime timers.
…tive gate, identified long-timer removal) Cancelled timers no longer leave stale wakeups. Two tiers by remaining delay, both preserving exact clear semantics from any thread (multithreaded JS can schedule and clear on non-looper threads): - short timers (<32ms): the token carries a native claim cell - a slot in a fixed per-loop atomic table indexed by timer id, with the id embedded in the cell word so cancellation can never hit a recycled cell. clearTimeout is a single native CAS (zero JNI): winning proves the token dead everywhere, so the sorted entry is erased outright; losing means dispatch owns the token, so a tombstone is left for it. EventLoopHandler claims cells through a @CriticalNative CAS (the annotation is public API in current SDKs; where ART doesn't apply it the method degrades to a plain JNI call with identical semantics) before entering the runtime, so a cancelled token dies in Java in nanoseconds - without acquiring the isolate Locker, which previously let a stale token park the main thread behind a long background JS turn. Only the gate retires cells, and cell tokens are never removeMessages()ed, so each cell sees exactly one gate pass; a busy slot (interval re-arm racing its previous token, or id collision beyond 1024 in-flight) just downgrades the token to plain+tombstone. - long timers (>=32ms, debounce territory): the token carries a Java AtomicBoolean peer, claimed in handleMessage. clearTimeout CASes the peer and on winning removeMessages()es the queued token: a cleared debounce timer produces no wakeup at all. The peer and its Message are GC-owned, which makes the removal-vs-in-flight-dequeue race harmless - a lost race costs at most one no-op wakeup, never an ordering violation. Below the cutoff a stale wakeup lands within two frames of the interaction that scheduled it (the app is provably awake), so the zero-allocation cell path applies instead. Only the newest token of an interval is cancellable; older tokens orphaned by a re-arm keep functioning anonymously through their own carriers, so token/slot parity holds under the anonymous-dispatch shuffle. SetTimer now converts a failed token post into a JS exception instead of unwinding a NativeScriptException through the V8 callback frame. Verified on device: ordering probes 100% across all scenarios (timer FIFO ties, clear-vs-Handler.post in both orders, orphan gap, triple-clear, clearInterval-from-callback, starvation), and the full suite (78 suites / 668 specs) green, including new specs for identified clear, background-thread clear racing dispatch, and interval stop.
…ks from review - @CriticalNative is ignored below API 26, where ART calls the method through the standard JNI ABI - binding the critical-convention function there would misread its arguments (minSdk is 21). Registration now binds a standard-ABI twin on api < 26, so the gate behaves identically on every supported API level. RegisterNatives failure no longer asserts: it clears the pending exception and gates PostTimerToken to plain tokens, so the unbound native can never be reached. - a failed JNI token post no longer leaks state: PostTimerToken releases the claim cell (no dispatch gate will ever retire it), and addTask erases the just-inserted sorted slot and map entry before rethrowing - a tokenless slot would otherwise consume another token's dispatch (live) or starve the item behind it (tombstoned). - RunOnMainThreadCallback resolves the main event loop before caching the callback, so a pre-init call can't pin the closure in the cache with no post to consume it. - tests: done.fail does not exist in the pinned jasmine 2.0.1 (it would TypeError inside the rejection handler and time out silently) - replaced with record-then-done; the background-clear race spec now counts only iterations whose clear provably ran (AtomicBoolean signal, bounded attempts), so it can't pass without racing. The RunMainThreadEntry isolate-liveness window flagged by review is byte-for-byte the removed pipe implementation's behavior and needs teardown-spanning liveness; deferred to the teardown-coordination work queued with the kExplicit follow-up.
|
Second CodeRabbit batch triaged; all verified against the code and addressed in the latest push except one, dispositioned below:
Full device suite re-run green after these changes. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Problem
Nothing pumps the V8 platform's foreground task queues. v8::platform::PumpMessageLoop only ran inside the WASM-scoped MessageLoopTimer (an ALooper fd fed by a detached thread polling every 100ms, wrapped around WebAssembly.compile/instantiate via JS proxies) and the inspector pause loops. Everything else V8 posts to its foreground runner just sat there:
Separately, the runtime had grown four bespoke implementations of "get work onto a runtime thread": TimerHandler tokens, LooperTasks' eventfd, the worker inbound eventfd queue, and (in this PR's first cut) another token Handler for platform tasks.
Change: a per-runtime EventLoop with two lanes
Each Runtime now owns an EventLoop (the Android analogue of the iOS runtime's ExecuteOnRunLoop seam), bound to its thread in PrepareV8Runtime. Work is routed by ordering contract:
Ordered lane — work whose ordering is observable against app-level Java messages. Rides the Java MessageQueue via a dedicated com.tns.EventLoopHandler using anonymous "task due" tokens (the Timers scheme from bfd7650), so it is strictly FIFO with Handler.post runnables and JS timers. 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 (WASM finalization, Atomics.waitAsync wakeups, GC tasks), worker→parent messages, unhandled-rejection drains. Rides an EFD_SEMAPHORE eventfd plus one timerfd (armed to the earliest delayed due time) on the thread's ALooper — Chrome's MessagePumpAndroid shape. No JNI on the post path, so V8's non-JVM worker threads post without attaching to the JVM. One eventfd unit = one unit of work per looper callback, so bursts interleave fairly with Java messages instead of draining in one go.
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 entry ends with a microtask checkpoint — work like the waitAsync wakeup resolves promises without entering JS, which kAuto's depth-0 drain never sees. Shutdown drops queued work and late posts, mirroring the old LooperTasks "message to a terminated runtime" semantics; leftover wakeups no-op like cleared-timer tokens.
Inspector pause loops (where the looper isn't polling) drain only nestable v8 tasks, bounded to the entries present at call time; non-nestable tasks and plain posts run from their own wakeups after the pause unwinds — matching the old PumpMessageLoop + LooperTasks behavior split.
Removed
Timers deliberately stays separate: it is the ordered lane specialized with sub-millisecond ordering machinery, and it is battle-tested.
Not in this PR
Microtask policy is untouched (kAuto). The cross-thread microtask-drain design (continuations always landing on the runtime thread under multithreaded JS) builds on this seam later.
Tests
Post-review hardening
An independent deep review of the scheduler surfaced two defects, both fixed here:
Also from review: the inspector-pause drain guards against C++ exceptions unwinding through v8 inspector frames, and fd callbacks ignore spurious wakeups (read failure) instead of consuming an entry.
Semantic deltas vs the old LooperTasks (deliberate): worker→parent messages now run one-per-looper-poll instead of batch-per-wakeup (Java messages interleave between them; relative order preserved), and each entry is followed by a Locker + microtask checkpoint. Open question flagged by review: microtask checkpoints currently run during debugger pauses (Blink parity) — see PR discussion.
Review sequencing, applied on this PR
Per the design review's sequencing (everything except the kExplicit microtask work, which remains a follow-up):
Timers merged into the ordered lane (with tombstones). TimerHandler is deleted; timers post anonymous tokens through the EventLoop, and the token drain runs the earliest due item across timers and ordered macrotasks — one due-ordered domain, still strictly FIFO with Handler.post. clearTimeout now leaves 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 Java messages between the two token positions — this also fixes the pre-existing congestion deviation in shipped timers. FireTimer's internals (sub-ms sorted list, interval catch-up, nesting clamp) are untouched; the check-and-run is a single RunIfEarliest call under one Locker acquisition, since background threads mutate timer bookkeeping via setTimeout under multithreaded JS.
__runOnMainThread promoted into the internal lane. The 2MB main-looper pipe is gone; closures ride bare internal-lane entries that skip the loop's Locker/checkpoint — the closure locks the caller's isolate, and taking the main isolate's Locker first would nest Lockers across isolates (deadlock-capable against worker→main entry paths, per review). Delivery stays one-per-poll like the old fd callback. Incidental fixes: the callback cache is now mutex-guarded (it was written from arbitrary threads whose different-isolate Lockers provided no mutual exclusion), and uncaught callback exceptions surface as pending Java exceptions instead of unwinding C++ through the ALooper frame.
Not applied: kExplicit microtask policy (excluded by request) and the internal-lane budgeted batch drain (the review gates it on profiling evidence; the old pipe was also one-per-poll, so there is no parity argument for it).
Cancellable timer tokens (wakeup hygiene for debounce workloads)
Tombstones fix ordering but leave a cleared timer's wakeup in the queue — a no-op that still wakes the looper at due time and, worse, acquires the isolate Locker (a stale token could park the main thread behind a long background JS turn under multithreaded JS). Cancelled timers now neutralize their token, in two tiers by remaining delay:
The cutoff is a fixed constant (32ms): timer delays cluster bimodally (0–16ms scheduling/animation vs ≥100ms debounce/timeouts), and the identified clear is a net lifetime JNI reduction (one clear-time crossing replaces a deferred full dispatch). Only the newest token of an interval is cancellable; older re-arm-orphaned tokens keep functioning anonymously, preserving token/slot parity under anonymous dispatch.
Verified on device: the orphan-token ordering probes pass 100% across every scenario (timer-FIFO ties, clear-vs-Handler.post in both orders, orphan-across-gap, triple-clear, clearInterval from its own callback, starvation after heavy clearing), and the full suite is green (78 suites / 668 specs) including new specs for identified clears, a background-thread clear racing dispatch (multithreaded JS), and interval stop.
Summary by CodeRabbit
New Features
Bug Fixes
Tests