feat: per-runtime EventLoop - v8 platform tasks + two-lane scheduler (Java MessageQueue / ALooper fd) - #2003
feat: per-runtime EventLoop - v8 platform tasks + two-lane scheduler (Java MessageQueue / ALooper fd)#2003edusperoni wants to merge 5 commits into
Conversation
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.
📝 WalkthroughWalkthroughThe runtime replaces legacy looper and timer infrastructure with per-isolate ChangesUnified event-loop refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
test-app/runtime/src/main/cpp/NativeScriptPlatform.h (1)
120-124: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider guarding the static JNI cache with
std::once_flag.Each
ForegroundTaskRunnerowns its ownmutex_, so the lock inBindToCurrentThreaddoes 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_oncewould 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
BindToCurrentThreadwithstd::call_once(EVENT_LOOP_HANDLER_INIT, ...).🤖 Prompt for AI Agents
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/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.
🤖 Prompt for all review comments with AI agents
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.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b8fba76-b1fd-4594-9322-720eefa43cf1
📒 Files selected for processing (13)
test-app/app/src/main/assets/app/mainpage.jstest-app/app/src/main/assets/app/tests/testEventLoop.jstest-app/runtime/CMakeLists.txttest-app/runtime/src/main/cpp/JsV8InspectorClient.cpptest-app/runtime/src/main/cpp/MessageLoopTimer.cpptest-app/runtime/src/main/cpp/MessageLoopTimer.htest-app/runtime/src/main/cpp/NativeScriptPlatform.cpptest-app/runtime/src/main/cpp/NativeScriptPlatform.htest-app/runtime/src/main/cpp/Runtime.cpptest-app/runtime/src/main/cpp/Runtime.htest-app/runtime/src/main/cpp/WorkerInspectorClient.cpptest-app/runtime/src/main/cpp/js/message-loop-timer.jstest-app/runtime/src/main/java/com/tns/EventLoopHandler.java
💤 Files with no reviewable changes (4)
- test-app/runtime/src/main/cpp/MessageLoopTimer.h
- test-app/runtime/src/main/cpp/js/message-loop-timer.js
- test-app/runtime/src/main/cpp/Runtime.h
- test-app/runtime/src/main/cpp/MessageLoopTimer.cpp
…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.
edusperoni
changed the title
|
Addressed the CodeRabbit findings; note the runner has since been restructured into a two-lane
|
…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).
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 Merge proposal (Timers + ordered lane into one token stream): endorsed, with one addition. Adversarial analysis confirmed a leftover Agreed sequencing: (a) this PR with the fixes above → (b) Timers/ordered-lane merge with tombstones → (c) Open questions for maintainers:
|
…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.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
test-app/runtime/src/main/cpp/CallbackHandlers.cpp (1)
809-819: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCatch
std::exceptionby const reference.Line 811 catches by value. The copy slices any derived exception, so
e.what()at Line 813 reports the basestd::exceptiontext instead of the real message. Catch byconst std::exception&.♻️ Proposed refactor
- } catch (std::exception e) { + } catch (const std::exception& e) { stringstream ss; ss << "Error: c++ exception: " << e.what() << endl;🤖 Prompt for AI Agents
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.cpp (1)
244-248: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse the
nowparameter instead of recomputing the time.
RunIfEarliestreceivesnow, then compares against a freshnow_ms()at Line 245. TheotherDuevalue was computed by the event loop against the passednow. Two different time bases make the "earliest across both domains" decision inconsistent: a timer whosedueTimefalls betweennowandnow_ms()becomes eligible while the loop treated no ordered entry as due. Use the parameter for one consistent basis.♻️ Proposed refactor
auto 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; }🤖 Prompt for AI Agents
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.test-app/runtime/src/main/cpp/Timers.h (1)
139-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign 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::RunIfEarliestinTimers.cppstates the opposite:sortedTimers_is mutated throughsetTimeoutfrom background threads under multithreaded JS, and the isolateLockeris the guard.OrderedTaskSourceinEventLoop.hdocuments the sameLocker-based contract. Update this comment so future readers do not remove theLockeracquisition.📝 Proposed comment fix
// 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.🤖 Prompt for AI Agents
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.
🤖 Prompt for all review comments with AI agents
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.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 176a9121-a7f6-46ca-bc75-1b507efbd79e
📒 Files selected for processing (23)
test-app/app/src/main/assets/app/tests/eventLoopEchoWorker.jstest-app/app/src/main/assets/app/tests/testEventLoop.jstest-app/runtime/CMakeLists.txttest-app/runtime/src/main/cpp/CallbackHandlers.cpptest-app/runtime/src/main/cpp/CallbackHandlers.htest-app/runtime/src/main/cpp/EventLoop.cpptest-app/runtime/src/main/cpp/EventLoop.htest-app/runtime/src/main/cpp/JsV8InspectorClient.cpptest-app/runtime/src/main/cpp/LooperTasks.cpptest-app/runtime/src/main/cpp/LooperTasks.htest-app/runtime/src/main/cpp/NativeScriptException.cpptest-app/runtime/src/main/cpp/NativeScriptException.htest-app/runtime/src/main/cpp/NativeScriptPlatform.cpptest-app/runtime/src/main/cpp/NativeScriptPlatform.htest-app/runtime/src/main/cpp/Runtime.cpptest-app/runtime/src/main/cpp/Runtime.htest-app/runtime/src/main/cpp/Timers.cpptest-app/runtime/src/main/cpp/Timers.htest-app/runtime/src/main/cpp/WorkerInspectorClient.cpptest-app/runtime/src/main/cpp/WorkerWrapper.cpptest-app/runtime/src/main/cpp/WorkerWrapper.htest-app/runtime/src/main/java/com/tns/EventLoopHandler.javatest-app/runtime/src/main/java/com/tns/TimerHandler.java
💤 Files with no reviewable changes (3)
- test-app/runtime/src/main/cpp/LooperTasks.h
- test-app/runtime/src/main/java/com/tns/TimerHandler.java
- test-app/runtime/src/main/cpp/LooperTasks.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- test-app/runtime/CMakeLists.txt
- test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp
- test-app/runtime/src/main/java/com/tns/EventLoopHandler.java
- test-app/runtime/src/main/cpp/WorkerInspectorClient.cpp
| }).catch(e => { | ||
| done.fail("promise chain failed: " + e); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
done.fail does not exist in the pinned Jasmine version.
This test app pins Jasmine 2.0.1. In that version done is a plain function with no .fail property. Line 60 therefore throws a TypeError inside the rejection handler. done is never called, the handler's own rejection goes unhandled, and the spec fails by timeout with no message. That is the same failure mode this handler was added to prevent.
Use the explicit two-handler form: assert on the error, then call done(). In Jasmine 2.0.1 addExpectationResult records the failure without throwing, so done() is always reached and the spec reports the real error.
💚 Proposed fix
- }).catch(e => {
- done.fail("promise chain failed: " + e);
- });
+ }).catch(e => {
+ expect(e).toBeUndefined();
+ done();
+ });Based on learnings: "done is a plain function with no .fail property, so done.fail is undefined", and "The correct async test pattern for this version is the explicit two-handler form rather than done.fail".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }).catch(e => { | |
| done.fail("promise chain failed: " + e); | |
| }); | |
| }).catch(e => { | |
| expect(e).toBeUndefined(); | |
| done(); | |
| }); |
🤖 Prompt for AI Agents
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/app/src/main/assets/app/tests/testEventLoop.js` around lines 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.
Source: Learnings
| { | ||
| std::lock_guard<std::mutex> lock(cacheMutex_); | ||
| bool inserted; | ||
| std::tie(std::ignore, inserted) = cache_.try_emplace(key, isolate, callback); | ||
| assert(inserted && "Main thread callback ID should not be duplicated"); | ||
| } | ||
|
|
||
| auto value = Callback(key); | ||
| auto size = sizeof(Callback); | ||
| auto wrote = write(Runtime::GetWriter(),&value , size); | ||
| auto mainLoop = Runtime::GetMainEventLoop(); | ||
| if (mainLoop == nullptr) { | ||
| return; | ||
| } | ||
| // bare entry: the closure locks the CALLER's isolate (possibly a | ||
| // worker's), so the loop must not take the main isolate's Locker first - | ||
| // nesting the two can deadlock against multithreaded-JS entry paths | ||
| mainLoop->PostInternalBare([key]() { RunMainThreadEntry(key); }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Insert into cache_ only after the event loop is available.
Lines 687-692 insert the entry. Lines 694-697 return when mainLoop == nullptr, and the entry stays in cache_ forever. Each such entry holds a v8::Global<v8::Function>, so the callback and its whole closure are retained until RemoveIsolateEntries runs for that isolate. Resolve the loop first, then insert.
🔧 Proposed fix
- {
- std::lock_guard<std::mutex> lock(cacheMutex_);
- bool inserted;
- std::tie(std::ignore, inserted) = cache_.try_emplace(key, isolate, callback);
- assert(inserted && "Main thread callback ID should not be duplicated");
- }
-
auto mainLoop = Runtime::GetMainEventLoop();
if (mainLoop == nullptr) {
return;
}
+
+ {
+ std::lock_guard<std::mutex> lock(cacheMutex_);
+ bool inserted;
+ std::tie(std::ignore, inserted) = cache_.try_emplace(key, isolate, callback);
+ assert(inserted && "Main thread callback ID should not be duplicated");
+ }
+
// bare entry: the closure locks the CALLER's isolate (possibly a
// worker's), so the loop must not take the main isolate's Locker first -
// nesting the two can deadlock against multithreaded-JS entry paths
mainLoop->PostInternalBare([key]() { RunMainThreadEntry(key); });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| std::lock_guard<std::mutex> lock(cacheMutex_); | |
| bool inserted; | |
| std::tie(std::ignore, inserted) = cache_.try_emplace(key, isolate, callback); | |
| assert(inserted && "Main thread callback ID should not be duplicated"); | |
| } | |
| auto value = Callback(key); | |
| auto size = sizeof(Callback); | |
| auto wrote = write(Runtime::GetWriter(),&value , size); | |
| auto mainLoop = Runtime::GetMainEventLoop(); | |
| if (mainLoop == nullptr) { | |
| return; | |
| } | |
| // bare entry: the closure locks the CALLER's isolate (possibly a | |
| // worker's), so the loop must not take the main isolate's Locker first - | |
| // nesting the two can deadlock against multithreaded-JS entry paths | |
| mainLoop->PostInternalBare([key]() { RunMainThreadEntry(key); }); | |
| auto mainLoop = Runtime::GetMainEventLoop(); | |
| if (mainLoop == nullptr) { | |
| return; | |
| } | |
| { | |
| std::lock_guard<std::mutex> lock(cacheMutex_); | |
| bool inserted; | |
| std::tie(std::ignore, inserted) = cache_.try_emplace(key, isolate, callback); | |
| assert(inserted && "Main thread callback ID should not be duplicated"); | |
| } | |
| // bare entry: the closure locks the CALLER's isolate (possibly a | |
| // worker's), so the loop must not take the main isolate's Locker first - | |
| // nesting the two can deadlock against multithreaded-JS entry paths | |
| mainLoop->PostInternalBare([key]() { RunMainThreadEntry(key); }); |
🤖 Prompt for AI Agents
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 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.
| void CallbackHandlers::RunMainThreadEntry(uint64_t key) { | ||
| Isolate *isolate; | ||
| { | ||
| std::lock_guard<std::mutex> lock(cacheMutex_); | ||
| auto it = cache_.find(key); | ||
| if (it == cache_.end()) { | ||
| return; | ||
| } | ||
| isolate = it->second.isolate_; | ||
| } | ||
|
|
||
| Isolate *isolate = it->second.isolate_; | ||
| v8::Locker locker(isolate); | ||
| Isolate::Scope isolate_scope(isolate); | ||
| HandleScope handle_scope(isolate); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate RemoveIsolateEntries call sites relative to isolate disposal.
set -euo pipefail
rg -nP -C 10 '\bRemoveIsolateEntries\s*\(' --glob '*.cpp' --glob '*.h'
# Disposal ordering in runtime teardown
rg -nP -C 12 '->\s*Dispose\s*\(\s*\)' --glob '*.cpp'
# DestroyRuntime body, which likely drives the cleanup order
fd -t f 'Runtime\.cpp$' --exec ast-grep run --lang cpp --pattern 'void Runtime::DestroyRuntime() { $$$ }' {}Repository: NativeScript/android
Length of output: 158
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(CallbackHandlers|WorkerWrapper|Runtime)\.(cpp|h)$' || true
printf '%s\n' '--- RemoveIsolateEntries references ---'
rg -n -C 12 '\bRemoveIsolateEntries\s*\(' . || true
printf '%s\n' '--- isolate disposal references ---'
rg -n -C 12 '\.(Dispose|dispose)\s*\(\s*\)|->\s*Dispose\s*\(\s*\)' . || trueRepository: NativeScript/android
Length of output: 9824
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- CallbackHandlers entry and cache cleanup ---'
sed -n '680,750p' test-app/runtime/src/main/cpp/CallbackHandlers.cpp
sed -n '1585,1645p' test-app/runtime/src/main/cpp/CallbackHandlers.cpp
printf '%s\n' '--- Worker teardown ---'
sed -n '430,490p' test-app/runtime/src/main/cpp/WorkerWrapper.cpp
printf '%s\n' '--- Runtime destructor and destroy path ---'
sed -n '260,315p' test-app/runtime/src/main/cpp/Runtime.cpp
rg -n -C 8 'DestroyRuntime|RunMainThreadEntry|cache_|IsolateDisposed|AddIsolate|Register' \
test-app/runtime/src/main/cpp/Runtime.cpp \
test-app/runtime/src/main/cpp/CallbackHandlers.cpp \
test-app/runtime/src/main/cpp/WorkerWrapper.cpp \
test-app/runtime/src/main/cpp/NativeScriptPlatform.cpp \
test-app/runtime/src/main/cpp/NativeScriptPlatform.h || trueRepository: NativeScript/android
Length of output: 31713
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
callback = Path("test-app/runtime/src/main/cpp/CallbackHandlers.cpp").read_text()
worker = Path("test-app/runtime/src/main/cpp/WorkerWrapper.cpp").read_text()
runtime = Path("test-app/runtime/src/main/cpp/Runtime.cpp").read_text()
def span(text, signature):
start = text.index(signature)
end = text.find("\n}", start)
return text[start:end + 2]
entry = span(callback, "void CallbackHandlers::RunMainThreadEntry(uint64_t key)")
teardown_start = worker.index("void WorkerWrapper::BackgroundLooper")
teardown = worker[teardown_start:]
destroy_start = runtime.index("void Runtime::DestroyRuntime()")
destroy = runtime[destroy_start:runtime.find("\n}", destroy_start) + 2]
destructor = span(runtime, "Runtime::~Runtime()")
checks = {
"entry reads isolate while cacheMutex_ is held": bool(re.search(
r'lock_guard<std::mutex> lock\(cacheMutex_\).*?cache_\.find\(key\).*?isolate = it->second\.isolate_;',
entry, re.S)),
"entry constructs Locker after releasing cacheMutex_": entry.index("v8::Locker locker(isolate)") >
entry.index("isolate = it->second.isolate_;"),
"worker disposes isolate before deleting Runtime": teardown.index("isolate->Dispose()") <
teardown.index("delete runtime_"),
"Runtime destructor removes callback entries": "CallbackHandlers::RemoveIsolateEntries(m_isolate)" in destructor,
"DestroyRuntime does not remove callback entries": "RemoveIsolateEntries" not in destroy,
}
for name, result in checks.items():
print(f"{'PASS' if result else 'FAIL'}: {name}")
print("\nRelevant teardown order:")
for token in ("runtime_->DestroyRuntime();", "isolate->Dispose();",
"NativeScriptPlatform::Instance()->IsolateDisposed",
"delete runtime_;"):
print(f"{token}: offset {teardown.index(token)}")
print(f"RemoveIsolateEntries in Runtime::~Runtime: offset {destructor.index('CallbackHandlers::RemoveIsolateEntries')}")
PYRepository: NativeScript/android
Length of output: 673
Protect the cached isolate until RunMainThreadEntry acquires its V8 lock
RunMainThreadEntry releases cacheMutex_ after reading the raw isolate. Worker teardown can then call isolate->Dispose() before Runtime::~Runtime removes the cache entry. v8::Locker can receive the freed pointer. The second lookup does not prevent this race. Use an ownership or liveness mechanism that spans the lookup and lock acquisition.
🤖 Prompt for AI Agents
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 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.
…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.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
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 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.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6de1b1b0-c38a-4bbb-9d5f-fff507bf1c32
📒 Files selected for processing (6)
test-app/app/src/main/assets/app/tests/testEventLoop.jstest-app/runtime/src/main/cpp/EventLoop.cpptest-app/runtime/src/main/cpp/EventLoop.htest-app/runtime/src/main/cpp/Timers.cpptest-app/runtime/src/main/cpp/Timers.htest-app/runtime/src/main/java/com/tns/EventLoopHandler.java
🚧 Files skipped from review as they are similar to previous changes (1)
- test-app/runtime/src/main/cpp/Timers.h
| new java.lang.Thread(new java.lang.Runnable({ | ||
| run() { | ||
| __ns__clearTimeout(t1); | ||
| } | ||
| })).start(); | ||
| handler.post(new java.lang.Runnable({ | ||
| run: () => order.push("java") | ||
| })); | ||
| __ns__setTimeout(() => { | ||
| order.push("t2"); | ||
| const observed = order.join(">"); | ||
| // t1 either fired before the clear landed (at its own legal | ||
| // slot, ahead of "java") or never; t2 must never jump "java" | ||
| expect(observed === "java>t2" || observed === "t1>java>t2").toBe(true); | ||
| if (--remaining === 0) { | ||
| done(); | ||
| } else { | ||
| iter(); | ||
| } | ||
| }, 5); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Confirm that the background-thread clear operation ran.
The test does not wait for __ns__clearTimeout(t1) to execute. The assertion can pass with "java>t2" while the background thread has not started. Add an AtomicBoolean or equivalent completion signal. Assert that the thread set it before calling done().
Proposed fix
const order = [];
+ const cleared = new java.util.concurrent.atomic.AtomicBoolean(false);
const handler = new android.os.Handler(android.os.Looper.myLooper());
const t1 = __ns__setTimeout(() => order.push("t1"), 0);
new java.lang.Thread(new java.lang.Runnable({
run() {
__ns__clearTimeout(t1);
+ cleared.set(true);
}
})).start();
@@
// slot, ahead of "java") or never; t2 must never jump "java"
expect(observed === "java>t2" || observed === "t1>java>t2").toBe(true);
+ expect(cleared.get()).toBe(true);
if (--remaining === 0) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| new java.lang.Thread(new java.lang.Runnable({ | |
| run() { | |
| __ns__clearTimeout(t1); | |
| } | |
| })).start(); | |
| handler.post(new java.lang.Runnable({ | |
| run: () => order.push("java") | |
| })); | |
| __ns__setTimeout(() => { | |
| order.push("t2"); | |
| const observed = order.join(">"); | |
| // t1 either fired before the clear landed (at its own legal | |
| // slot, ahead of "java") or never; t2 must never jump "java" | |
| expect(observed === "java>t2" || observed === "t1>java>t2").toBe(true); | |
| if (--remaining === 0) { | |
| done(); | |
| } else { | |
| iter(); | |
| } | |
| }, 5); | |
| const order = []; | |
| const cleared = new java.util.concurrent.atomic.AtomicBoolean(false); | |
| const handler = new android.os.Handler(android.os.Looper.myLooper()); | |
| const t1 = __ns__setTimeout(() => order.push("t1"), 0); | |
| new java.lang.Thread(new java.lang.Runnable({ | |
| run() { | |
| __ns__clearTimeout(t1); | |
| cleared.set(true); | |
| } | |
| })).start(); | |
| handler.post(new java.lang.Runnable({ | |
| run: () => order.push("java") | |
| })); | |
| __ns__setTimeout(() => { | |
| order.push("t2"); | |
| const observed = order.join(">"); | |
| // t1 either fired before the clear landed (at its own legal | |
| // slot, ahead of "java") or never; t2 must never jump "java" | |
| expect(observed === "java>t2" || observed === "t1>java>t2").toBe(true); | |
| expect(cleared.get()).toBe(true); | |
| if (--remaining === 0) { | |
| done(); | |
| } else { | |
| iter(); | |
| } | |
| }, 5); |
🤖 Prompt for AI Agents
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/app/src/main/assets/app/tests/testEventLoop.js` around lines 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.
| EVENT_LOOP_HANDLER_POST_TOKEN = | ||
| env.GetMethodID(EVENT_LOOP_HANDLER_CLASS, "postToken", "(JII)V"); | ||
| EVENT_LOOP_HANDLER_POST_IDENTIFIED = env.GetMethodID( | ||
| EVENT_LOOP_HANDLER_CLASS, "postIdentified", "(J)Ljava/lang/Object;"); | ||
| EVENT_LOOP_HANDLER_CANCEL_IDENTIFIED = env.GetMethodID( | ||
| EVENT_LOOP_HANDLER_CLASS, "cancelIdentified", "(Ljava/lang/Object;)Z"); | ||
| EVENT_LOOP_HANDLER_RELEASE = env.GetMethodID(EVENT_LOOP_HANDLER_CLASS, "release", "()V"); | ||
| // the @CriticalNative gate must be bound explicitly (name resolution | ||
| // doesn't apply to the critical calling convention on older ART) | ||
| static const JNINativeMethod claimMethod = { | ||
| const_cast<char*>("nativeClaimToken"), const_cast<char*>("(JJ)Z"), | ||
| reinterpret_cast<void*>(EventLoop::ClaimTokenCritical)}; | ||
| JNIEnv* rawEnv = env; | ||
| jint registered = rawEnv->RegisterNatives(EVENT_LOOP_HANDLER_CLASS, &claimMethod, 1); | ||
| assert(registered == 0); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Android CriticalNative RegisterNatives requirements no JNIEnv parameter API level
💡 Result:
When using the @CriticalNative annotation in Android, the native implementation is required to exclude both the JNIEnv and jclass parameters from its function signature [1][2]. This change in the JNI transition ABI is necessary to enable the performance optimizations associated with @CriticalNative [1][3]. Regarding JNI registration, it is strongly recommended to explicitly register these methods using JNI RegisterNatives [1][2]. While dynamic linking might work on Android 12+, explicit registration is strictly required for compatibility with Android versions 8 through 11 [1][4]. Furthermore, @CriticalNative methods must not use managed objects in their parameters, return values, or as an implicit this [1][2]. Because the annotation is ignored on Android 7 and lower, maintaining the standard JNI signature (with JNIEnv and jclass) on those versions would be necessary to avoid ABI mismatches, which can lead to incorrect argument marshalling and crashes [1][5].
Citations:
- 1: https://developer.android.com/reference/dalvik/annotation/optimization/CriticalNative
- 2: https://developer.android.com/ndk/guides/jni-tips
- 3: https://android.googlesource.com/platform/libcore/+/main/dalvik/src/main/java/dalvik/annotation/optimization/CriticalNative.java
- 4: https://android.googlesource.com/platform/libcore/+/master/dalvik/src/main/java/dalvik/annotation/optimization/CriticalNative.java
- 5: https://developer.android.com/reference/kotlin/dalvik/annotation/optimization/CriticalNative
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate file map ---'
git ls-files 'test-app/runtime/src/main/cpp/EventLoop.cpp' 'test-app/**' | head -80
printf '%s\n' '--- relevant source ---'
nl -ba test-app/runtime/src/main/cpp/EventLoop.cpp | sed -n '1,180p'
printf '%s\n' '--- related declarations and call sites ---'
rg -n -C 3 'ClaimTokenCritical|nativeClaimToken|CriticalNative|RegisterNatives|EVENT_LOOP_HANDLER_CLASS|ClaimToken' test-app
printf '%s\n' '--- Android API configuration ---'
rg -n -C 3 'minSdk|minSdkVersion|ANDROID_PLATFORM|compileSdk|targetSdk|API_LEVEL|apiLevel' . -g 'build.gradle*' -g '*.gradle' -g 'gradle.properties' -g 'CMakeLists.txt' -g '*.mk' -g '*.xml' | head -240Repository: NativeScript/android
Length of output: 29243
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- EventLoop binding and token paths ---'
sed -n '80,135p' test-app/runtime/src/main/cpp/EventLoop.cpp
sed -n '250,370p' test-app/runtime/src/main/cpp/EventLoop.cpp
printf '%s\n' '--- EventLoopHandler implementation ---'
sed -n '1,175p' test-app/runtime/src/main/java/com/tns/EventLoopHandler.java
printf '%s\n' '--- build constants and manifest ---'
rg -n -C 4 'NS_DEFAULT_MIN_SDK_VERSION|DEFAULT_MIN_SDK|uses-sdk|minSdkVersion' test-app -g '*.gradle' -g '*.properties' -g 'AndroidManifest.xml' -g '*.xml'
printf '%s\n' '--- all EventLoop token posting calls ---'
rg -n -C 5 'PostToken|postToken|cellWord|claimCell|ClaimCell|CancelClaim' test-app/runtime/src/main/cpp test-app/runtime/src/main/javaRepository: NativeScript/android
Length of output: 31162
🌐 Web query:
site:developer.android.com @CriticalNative API 26 RegisterNatives dynamic lookup Android 8 11 JNI
💡 Result:
When using the @CriticalNative annotation on Android, explicit registration via RegisterNatives is strictly required for compatibility with Android versions 8 through 11 [1][2][3]. Key technical details regarding this requirement: 1. Dynamic Lookup Support: The Android Runtime (ART) does not support dynamic (name-based) discovery of native methods annotated with @CriticalNative on Android versions 8 through 10 [1][3]. In Android 11, the dynamic lookup mechanism for these annotated methods is known to be buggy [1][3]. Relying on dynamic lookup instead of RegisterNatives on these versions is highly likely to result in application crashes [1][3]. 2. Mandatory Explicit Registration: To use @CriticalNative safely on devices running Android 8-11, you must manually register the native methods using the JNI RegisterNatives function, typically within JNI_OnLoad [1][2]. This bypasses the faulty or unimplemented dynamic lookup mechanisms for these specific optimized methods [1][3]. 3. ABI and Annotation Behavior: The @CriticalNative annotation changes the JNI transition Application Binary Interface (ABI), requiring the native implementation to omit the JNIEnv and jclass parameters [2]. If the annotation is present but the runtime fails to process it correctly (or ignores it, as happens on Android 7 and lower), a mismatch in the expected function signature will occur, leading to incorrect argument marshalling and likely crashes [1][2]. 4. Broader Support: While dynamic JNI linking for @CriticalNative methods became officially functional starting in Android 12, explicit registration with RegisterNatives remains the strongly recommended best practice for all versions to ensure optimal performance and compatibility [1][2][3].
Citations:
- 1: https://developer.android.com/ndk/guides/jni-tips
- 2: https://developer.android.com/reference/dalvik/annotation/optimization/CriticalNative
- 3: https://developer.android.com/training/articles/perf-jni
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- optimized-native API gating ---'
rg -n -C 6 'SUPPORTS_OPTIMIZED_NATIVE|SDK_INT|Legacy|CriticalNative|FastNative' test-app/runtime/src/main/java/com/tns/Runtime.java test-app/runtime/src/main/cpp/com_tns_Runtime.cpp
printf '%s\n' '--- runtime initialization and registration ---'
sed -n '1,75p' test-app/runtime/src/main/cpp/com_tns_Runtime.cpp
rg -n -C 5 'JNI_OnLoad|RegisterNatives|EventLoop::BindToCurrentThread|BindToCurrentThread' test-app/runtime/src/main/cpp test-app/runtime/src/main/java
printf '%s\n' '--- timer token consumers ---'
sed -n '110,180p' test-app/runtime/src/main/cpp/Timers.cpp
rg -n -C 4 'PostTimerToken|tokenCell_|tokenPeer_' test-app/runtime/src/main/cpp/Timers.cpp test-app/runtime/src/main/cpp/*.hRepository: NativeScript/android
Length of output: 47963
Handle registration failure and gate claim tokens
Clearing the pending exception does not create a plain-token fallback. PostTimerToken still emits non-zero cell words, and EventLoopHandler.handleMessage still calls nativeClaimToken. The project supports API 21, but @CriticalNative uses the standard JNI ABI before API 26. Add an API- and registration-capability-gated legacy path, and emit plain tokens when the critical binding is unavailable. Do not rely on name lookup because this implementation is provided only through RegisterNatives.
🧰 Tools
🪛 Cppcheck (2.21.0)
[error] 108-108: #error Unsupported bitness
(preprocessorErrorDirective)
🤖 Prompt for AI Agents
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/EventLoop.cpp` around lines 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.
| uint64_t word = 0; | ||
| auto& cell = claimCells_[((uint32_t) timerId) & (kClaimCells - 1)]; | ||
| uint64_t expected = 0; | ||
| uint64_t candidate = (((uint64_t) (uint32_t) timerId) << 2) | kCellActive; | ||
| if (cell.compare_exchange_strong(expected, candidate, std::memory_order_acq_rel)) { | ||
| word = candidate; | ||
| } | ||
| // a busy slot (previous token of the same interval still in flight, or an | ||
| // id collision) downgrades this token to plain; clear then uses tombstones | ||
| JEnv env; | ||
| env.CallVoidMethod(handler_, EVENT_LOOP_HANDLER_POST_TOKEN, uptimeMillis, | ||
| (jint) (word >> 32), (jint) (word & 0xffffffffull)); | ||
| return word; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A failed token post leaks the claim cell permanently.
Line 288 moves the cell to id|ACTIVE before the JNI post. Only the dispatch gate stores 0 again, and the gate runs only for a token that reached the queue. If env.CallVoidMethod at Line 294 throws (Timers::SetTimer now catches NativeScriptException from this path, see test-app/runtime/src/main/cpp/Timers.cpp Lines 263-270), no token exists, so the cell stays ACTIVE for the process lifetime. Every later timer whose id maps to that slot then downgrades to a plain token.
Release the cell when the post does not complete.
🛡️ Proposed fix
// a busy slot (previous token of the same interval still in flight, or an
// id collision) downgrades this token to plain; clear then uses tombstones
JEnv env;
- env.CallVoidMethod(handler_, EVENT_LOOP_HANDLER_POST_TOKEN, uptimeMillis,
- (jint) (word >> 32), (jint) (word & 0xffffffffull));
+ try {
+ env.CallVoidMethod(handler_, EVENT_LOOP_HANDLER_POST_TOKEN, uptimeMillis,
+ (jint) (word >> 32), (jint) (word & 0xffffffffull));
+ } catch (...) {
+ // no token reached the queue, so no dispatch gate will retire the cell
+ if (word != 0) {
+ cell.store(0, std::memory_order_release);
+ }
+ throw;
+ }
return word;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint64_t word = 0; | |
| auto& cell = claimCells_[((uint32_t) timerId) & (kClaimCells - 1)]; | |
| uint64_t expected = 0; | |
| uint64_t candidate = (((uint64_t) (uint32_t) timerId) << 2) | kCellActive; | |
| if (cell.compare_exchange_strong(expected, candidate, std::memory_order_acq_rel)) { | |
| word = candidate; | |
| } | |
| // a busy slot (previous token of the same interval still in flight, or an | |
| // id collision) downgrades this token to plain; clear then uses tombstones | |
| JEnv env; | |
| env.CallVoidMethod(handler_, EVENT_LOOP_HANDLER_POST_TOKEN, uptimeMillis, | |
| (jint) (word >> 32), (jint) (word & 0xffffffffull)); | |
| return word; | |
| uint64_t word = 0; | |
| auto& cell = claimCells_[((uint32_t) timerId) & (kClaimCells - 1)]; | |
| uint64_t expected = 0; | |
| uint64_t candidate = (((uint64_t) (uint32_t) timerId) << 2) | kCellActive; | |
| if (cell.compare_exchange_strong(expected, candidate, std::memory_order_acq_rel)) { | |
| word = candidate; | |
| } | |
| // a busy slot (previous token of the same interval still in flight, or an | |
| // id collision) downgrades this token to plain; clear then uses tombstones | |
| JEnv env; | |
| try { | |
| env.CallVoidMethod(handler_, EVENT_LOOP_HANDLER_POST_TOKEN, uptimeMillis, | |
| (jint) (word >> 32), (jint) (word & 0xffffffffull)); | |
| } catch (...) { | |
| // no token reached the queue, so no dispatch gate will retire the cell | |
| if (word != 0) { | |
| cell.store(0, std::memory_order_release); | |
| } | |
| throw; | |
| } | |
| return word; |
🤖 Prompt for AI Agents
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/EventLoop.cpp` around lines 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.
| try { | ||
| thiz->addTask(task); | ||
| } catch (NativeScriptException &e) { | ||
| // a failed JNI token post must surface as a JS exception, not | ||
| // unwind through the V8 callback frame | ||
| e.ReThrowToV8(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A throwing token post leaves partially committed state in two places. Both sites commit state before the JNI post and neither undoes it when the post throws, so a single failed post breaks the tokens-to-slots 1:1 invariant and burns a claim cell.
test-app/runtime/src/main/cpp/Timers.cpp#L263-L270: wrap thepostTimercall inaddTaskso a throw removes thetimerMap_entry and thesortedTimers_slot before the exception reaches this catch.test-app/runtime/src/main/cpp/EventLoop.cpp#L284-L296: store 0 back into the claim cell ifenv.CallVoidMethodthrows, because no token reaches the dispatch gate that would otherwise retire the cell.
📍 Affects 2 files
test-app/runtime/src/main/cpp/Timers.cpp#L263-L270(this comment)test-app/runtime/src/main/cpp/EventLoop.cpp#L284-L296
🤖 Prompt for AI Agents
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 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.
1 participant
Problem
Nothing pumps the V8 platform's foreground task queues.
v8::platform::PumpMessageLooponly ran inside the WASM-scopedMessageLoopTimer(anALooperfd fed by a detached thread polling every 100ms, wrapped aroundWebAssembly.compile/instantiatevia JS proxies) and the inspector pause loops. Everything else V8 posts to its foreground runner just sat there:Atomics.waitAsyncpromises never resolved (their wakeup is a foreground task).Separately, the runtime had grown four bespoke implementations of "get work onto a runtime thread":
TimerHandlertokens,LooperTasks' eventfd, the worker inbound eventfd queue, and (in this PR's first cut) another token Handler for platform tasks.Change: a per-runtime
EventLoopwith two lanesEach
Runtimenow owns anEventLoop(the Android analogue of the iOS runtime'sExecuteOnRunLoopseam), bound to its thread inPrepareV8Runtime. Work is routed by ordering contract:Ordered lane — work whose ordering is observable against app-level Java messages. Rides the Java
MessageQueuevia a dedicatedcom.tns.EventLoopHandlerusing anonymous "task due" tokens (theTimersscheme from bfd7650), so it is strictly FIFO withHandler.postrunnables 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.waitAsyncwakeups, GC tasks), worker→parent messages, unhandled-rejection drains. Rides anEFD_SEMAPHOREeventfd plus one timerfd (armed to the earliest delayed due time) on the thread'sALooper— Chrome'sMessagePumpAndroidshape. 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.NativeScriptPlatformwraps the default platform (workers/jobs/time/tracing delegate to libplatform) and servesGetForegroundTaskRunner(isolate)from the isolate'sEventLoop. The loop starts unbound and buffers (v8 requests the runner duringIsolate::New); binding flushes. Each executed entry ends with a microtask checkpoint — work like thewaitAsyncwakeup resolves promises without entering JS, whichkAuto's depth-0 drain never sees. Shutdown drops queued work and late posts, mirroring the oldLooperTasks"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+LooperTasksbehavior split.Removed
MessageLoopTimer(polling thread, pipe,WebAssemblyproxies inmessage-loop-timer.js) — async WASM promises now resolve promptly with no start/stop windows.LooperTasks— consolidated into the internal lane; call sites (worker messaging, exception drains) ported 1:1 including theweak_ptrchild semantics and drop-after-shutdown behavior.Timersdeliberately 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
testEventLoop.js:Atomics.waitAsyncnotify/timeout/mismatch + promise-chain ordering (the async cases hang without this change), plus ordered-lane specs:__ns__queueMacrotaskasync delivery, runs-after-microtasks, FIFO interleaving withsetTimeout(0), TypeError on non-function.Post-review hardening
An independent deep review of the scheduler surfaced two defects, both fixed here:
Atomics.waitAsynctimeout.~Runtime, which runs several JNI calls afterIsolate::Disposefrees the address — a concurrently created worker isolate could reuse the pointer and inherit the dead runtime's stopped loop (dropping all its work), and the late destructor could then evict the new tenant's entry. Fixes: matched erase (only removes the entry while it still maps to the disposing runtime's loop) immediately afterDispose,PrepareV8Runtimerefreshes 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.Also from review: the inspector-pause drain guards against C++ exceptions unwinding through v8 inspector frames, and fd callbacks ignore spurious wakeups (
readfailure) 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 aLocker+ 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
kExplicitmicrotask work, which remains a follow-up):Timers merged into the ordered lane (with tombstones).
TimerHandleris deleted; timers post anonymous tokens through theEventLoop, and the token drain runs the earliest due item across timers and ordered macrotasks — one due-ordered domain, still strictly FIFO withHandler.post.clearTimeoutnow 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 singleRunIfEarliestcall under one Locker acquisition, since background threads mutate timer bookkeeping viasetTimeoutunder multithreaded JS.__runOnMainThreadpromoted 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:
kExplicitmicrotask 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:
clearTimeoutis a single native CAS — zero JNI: winning proves the token dead (sorted entry erased outright); losing leaves a tombstone for the in-flight token.EventLoopHandlerclaims cells via a@CriticalNativeCAS (public API in current SDKs; degrades to plain JNI with identical semantics where unapplied) before entering the runtime — a cancelled token dies in Java in nanoseconds, never touching the Locker. Cells see exactly one gate pass by construction (only the gate retires; cell tokens are never removed), and a busy slot just downgrades to plain+tombstone.AtomicBooleanpeer claimed inhandleMessage; clearing CASes it and, on winning,removeMessages()es the queued token — a cleared debounce timer produces no wakeup at all. The CAS makes the removal-vs-in-flight race harmless: a lost race costs one no-op wakeup, never an ordering violation. Below the cutoff a stale wakeup lands within two frames of the interaction that scheduled it (app provably awake), so the zero-allocation path applies.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.postin both orders, orphan-across-gap, triple-clear,clearIntervalfrom 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