Repository navigation
refactor(runtime): a Runtime owns one isolate and publishes it from Init - #513
edusperoni wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (3 skipped: 3 … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly describes the main runtime change: Runtime creates and owns one isolate, then publishes it during Init.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (3 skipped: 3 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks the runtime’s start,
With limits tucked beside its heart.
The isolate wakes, then joins the line, Init() sets the caches in time.
Worker IDs arrive in tow,
And off the little runtimes go.
Comment @coderabbitai help to get the list of available commands.
Setting up a runtime took CreateIsolate, Init and, for workers, SetWorkerId, assembled by hand at each call site. The isolate joined the live registry in CreateIsolate, before Init wrote its Caches slot, so a thread that found it there (the memory-pressure handler, #512) could read that slot while it was being written. CreateIsolate could also be called twice on one Runtime, and a Runtime deleted before Init left its isolate registered and never disposed. - Runtime(const RuntimeOptions&) creates the isolate, homed on the calling thread, and takes the limits and worker id up front, so IsWorker() is correct during a worker's Init. The caller still locks the isolate and can set it up (WatchHeapLimit) before calling Init(). - Init() publishes the isolate to the registry only after writing its Caches slot. With the teardown already unregistering before clearing it, a thread that finds an isolate in the registry may read its slots under the lock. - A Runtime deleted before Init disposes its unpublished isolate. - CreateIsolate and SetWorkerId are gone. Reported in #512.
66ac3ce to
ca7dda6
Compare
|
Glad to help |
Supersedes #512, which found the race this fixes. Thanks @adrian-niculescu.
Problem
Setting up a runtime was assembled by hand at each call site:
NativeScript.mm):new Runtime(),CreateIsolate(), Locker,Init(isolate).Worker.mm): the same, plusWatchHeapLimitbeforeInit(isolate, true)andSetWorkerIdafter it.That split caused several problems:
CreateIsolateadded the isolate to the live registry (isolates_) beforeInitwrote itsCachesslot.Runtime::NotifyMemoryPressure(feat(runtime): report native memory held by wrappers to V8, forward memory pressure #507) runs on a dispatch queue and reads that slot for every registered isolate, so a memory-pressure event during startup could read it while it was being written. On a miss it also built a stand-inCachesoff-thread. fix(runtime): publish an isolate's Caches under the registry lock #512 fixed this by writing the slot under the registry lock; this PR removes the window instead.SetWorkerIdran afterInit, soRuntime::IsWorker()returned false during a worker'sInit. That includes worker-scoped class naming inClassBuilderand the main-isolate-only checks inns:runtime.CreateIsolatecould be called more than once on the sameRuntime. And aRuntimedeleted beforeInitleft its isolate registered and never disposed, becauseisolate_was only set at the end ofInit.Change
Runtime(const RuntimeOptions&)creates the isolate on the calling thread, which becomes its home thread. The isolate limits and the worker id are passed in up front, soIsWorker()is correct from the start.WatchHeapLimitthere. It then callsInit().Init()adds the isolate to the registry only after writing itsCachesslot. Teardown already removes it from the registry before clearing the slots. So any thread that finds an isolate in the registry may read its slots while holding the registry lock. This rule is documented onisolates_, and the memory-pressure handler relies on it unchanged.Runtimedeleted beforeInitdisposes its isolate, which no other thread has seen yet.CreateIsolateandSetWorkerIdare removed.Runtime.his a project header, not part of the public framework headers.Not covered
A reader that finds the isolate in the registry, releases the lock, and only then reads the slot can still race teardown's
Caches::Remove. The pre-LockerIsolateWrapper::IsValid()inArgConverter::MethodCallbackdoes this. That's a separate fix: read the slot only under the isolate's Locker.Tests
No new test: the startup overlap depends on timing (see #512 for a sleep-based repro). The full suite under ASan on a dedicated simulator gives 1835 specs, 0 failures, and no sanitizer reports.
Summary by CodeRabbit