feat: reloadApplication for JS bundle restart without restarting app process#384
feat: reloadApplication for JS bundle restart without restarting app process#384NathanWalker wants to merge 2 commits into
Conversation
…process Helpful for programmatic reset of JS isolate for clean restart of JS application as well as OTA (over-the-air) updates without restarting the entire app process.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds JavaScript-triggered application reloads through a native bridge, preserves runtime configuration for restart, exposes reload state to JavaScript, and prevents callbacks from using disposed isolates after reloads. ChangesRuntime reload flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant JavaScript as global.NativeScriptRuntime
participant RuntimeAPI as tns::InvokeReloadApplicationHook
participant ObjectiveC as NativeScriptRuntime
participant MainQueue as Main queue
participant NativeScript as NativeScript runtime
JavaScript->>RuntimeAPI: reloadApplication(baseDir?)
RuntimeAPI->>ObjectiveC: forward baseDir or nil
ObjectiveC->>MainQueue: schedule restart with copied Config
MainQueue->>NativeScript: shut down and re-initialize
MainQueue->>NativeScript: runMainApplication
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@NativeScript/NativeScript.mm`:
- Around line 182-184: The reload path in NativeScript’s restart flow still
carries over the process-global jsErrorOccurred flag, which can make the next
bundle start in the old debug error loop. Update the restart sequence around
currentNativeScript restartWithConfig: and runMainApplication to clear or
reinitialize jsErrorOccurred before launching the new isolate, so a fresh reload
starts from a clean JS error state.
In `@NativeScript/runtime/Runtime.mm`:
- Around line 577-588: Serialize access to the process-global
reloadApplicationHook_ used by SetReloadApplicationHook and
InvokeReloadApplicationHook: protect both reads and writes with synchronization
so initialization and concurrent JavaScript-thread calls cannot race. Update
InvokeReloadApplicationHook to copy the current hook under the lock, then
release the lock before invoking it so user code runs outside the critical
section and avoids deadlocks or lock contention.
🪄 Autofix (Beta)
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
Run ID: 7e5e1d22-c25d-42ab-b77f-3fcaea2ea84b
📒 Files selected for processing (4)
NativeScript/NativeScript.hNativeScript/NativeScript.mmNativeScript/runtime/Runtime.hNativeScript/runtime/Runtime.mm
| dispatch_async(dispatch_get_main_queue(), ^{ | ||
| [currentNativeScript restartWithConfig:config]; | ||
| [currentNativeScript runMainApplication]; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reset stale JS error state before starting the new bundle.
jsErrorOccurred is process-global and survives reloads. After a previous debug error, the new runMainApplication can enter the debug error loop even if the reload was meant to provide a clean isolate restart.
Proposed fix
dispatch_async(dispatch_get_main_queue(), ^{
+ tns::jsErrorOccurred = false;
[currentNativeScript restartWithConfig:config];
[currentNativeScript runMainApplication];
});📝 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.
| dispatch_async(dispatch_get_main_queue(), ^{ | |
| [currentNativeScript restartWithConfig:config]; | |
| [currentNativeScript runMainApplication]; | |
| dispatch_async(dispatch_get_main_queue(), ^{ | |
| tns::jsErrorOccurred = false; | |
| [currentNativeScript restartWithConfig:config]; | |
| [currentNativeScript runMainApplication]; |
🤖 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 `@NativeScript/NativeScript.mm` around lines 182 - 184, The reload path in
NativeScript’s restart flow still carries over the process-global
jsErrorOccurred flag, which can make the next bundle start in the old debug
error loop. Update the restart sequence around currentNativeScript
restartWithConfig: and runMainApplication to clear or reinitialize
jsErrorOccurred before launching the new isolate, so a fresh reload starts from
a clean JS error state.
| static ReloadApplicationHook reloadApplicationHook_; | ||
|
|
||
| void SetReloadApplicationHook(ReloadApplicationHook hook) { | ||
| reloadApplicationHook_ = std::move(hook); | ||
| } | ||
|
|
||
| bool InvokeReloadApplicationHook(const std::string& baseDir) { | ||
| if (!reloadApplicationHook_) { | ||
| return false; | ||
| } | ||
| return reloadApplicationHook_(baseDir); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all reload hook access sites and whether they can be reached from runtime initialization / JS callbacks.
rg -n -C4 '\b(SetReloadApplicationHook|InvokeReloadApplicationHook|DefineNativeScriptRuntime)\b' NativeScriptRepository: NativeScript/ios
Length of output: 5712
🏁 Script executed:
#!/bin/bash
# Check if <mutex> is included in Runtime.mm
head -50 NativeScript/runtime/Runtime.mm | rg "include.*mutex"Repository: NativeScript/ios
Length of output: 171
Serialize access to the global reload hook.
reloadApplicationHook_ is a process-global std::function modified by SetReloadApplicationHook during initialization while InvokeReloadApplicationHook may be called concurrently from JavaScript execution threads. Read and write access must be synchronized, and the hook invocation must occur outside the lock to prevent deadlocks or holding locks during user code execution.
Proposed fix
static ReloadApplicationHook reloadApplicationHook_;
+static std::mutex reloadApplicationHookMutex_;
void SetReloadApplicationHook(ReloadApplicationHook hook) {
+ std::lock_guard<std::mutex> lock(reloadApplicationHookMutex_);
reloadApplicationHook_ = std::move(hook);
}
bool InvokeReloadApplicationHook(const std::string& baseDir) {
- if (!reloadApplicationHook_) {
+ ReloadApplicationHook hook;
+ {
+ std::lock_guard<std::mutex> lock(reloadApplicationHookMutex_);
+ hook = reloadApplicationHook_;
+ }
+ if (!hook) {
return false;
}
- return reloadApplicationHook_(baseDir);
+ return hook(baseDir);
}📝 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.
| static ReloadApplicationHook reloadApplicationHook_; | |
| void SetReloadApplicationHook(ReloadApplicationHook hook) { | |
| reloadApplicationHook_ = std::move(hook); | |
| } | |
| bool InvokeReloadApplicationHook(const std::string& baseDir) { | |
| if (!reloadApplicationHook_) { | |
| return false; | |
| } | |
| return reloadApplicationHook_(baseDir); | |
| } | |
| static ReloadApplicationHook reloadApplicationHook_; | |
| static std::mutex reloadApplicationHookMutex_; | |
| void SetReloadApplicationHook(ReloadApplicationHook hook) { | |
| std::lock_guard<std::mutex> lock(reloadApplicationHookMutex_); | |
| reloadApplicationHook_ = std::move(hook); | |
| } | |
| bool InvokeReloadApplicationHook(const std::string& baseDir) { | |
| ReloadApplicationHook hook; | |
| { | |
| std::lock_guard<std::mutex> lock(reloadApplicationHookMutex_); | |
| hook = reloadApplicationHook_; | |
| } | |
| if (!hook) { | |
| return false; | |
| } | |
| return hook(baseDir); | |
| } |
🤖 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 `@NativeScript/runtime/Runtime.mm` around lines 577 - 588, Serialize access to
the process-global reloadApplicationHook_ used by SetReloadApplicationHook and
InvokeReloadApplicationHook: protect both reads and writes with synchronization
so initialization and concurrent JavaScript-thread calls cannot race. Update
InvokeReloadApplicationHook to copy the current hook under the lock, then
release the lock before invoking it so user code runs outside the critical
section and avoids deadlocks or lock contention.
|
I don't see this ever being a good idea. Bundle 1 calls UIApplicationMain with a custom AppDelegate extended in JS |
Yeah, the delegate class outlives the isolate, UIKit keeps dispatching to the old instance, and MethodCallback bails when the isolate's gone. So a JS-extended AppDelegate from bundle 1 would go inert after reload. The existing teardown behavior (same IsolateWrapper checks in ClassBuilder, the adapters, etc), and restartWithConfig itself has been in the runtime since the embedding work in #231. This just makes it callable from JS with the original config kept around. The reason it holds up in practice is that core doesn't actually rely on delegate dispatch for lifecycle, everything goes through NSNotificationCenter observers that get re-registered when the new isolate boots, and the soft-reboot path recreates the window against the live scene. We've been shipping these PR changes in production this year and it's been solid on this path, so this is proven useful already. The case you're describing (custom delegate methods like push token callbacks or openURL) is something worth handling though. Two things I can do as follow-up: have the soft-reboot path reassign UIApplication.sharedApplication.delegate to a fresh instance from the new bundle (with a strong ref since UIKit won't hold one), and add a log behind the script-loading flag when a dead-isolate callback fires so this failure mode is visible instead of silent. If you can think of other dispatch paths that'd stay pinned to the old isolate, lemme know. |
|
For now. With UIScene we now use delegates, so this only works until iOS 27. It works in embedding because in embedded we don't bootstrap the app and just render views on existing apps |
Instances of classes built by a previous isolate (e.g. a JS-extended UIApplicationDelegate or UISceneDelegate) outlive the isolate, and UIKit keeps dispatching to them after a soft reboot: - log dropped method callbacks (class + selector) behind logScriptLoading in ArgConverter::MethodCallback so the failure mode is visible instead of a silent no-op - add the same IsolateWrapper validity check to JS-implemented property getters/setters in ClassBuilder, which previously locked the disposed isolate (crash when UIKit queried `window` on a stale delegate) - expose NativeScriptRuntime.reloadCount, incremented by restartWithConfig, so @nativescript/core can detect a reloaded instance and reattach the UIApplication delegate and UIScene delegates from the new bundle
|
Thanks again for all the feedback @edusperoni, I've pushed up the follow-ups to address all cases (modern UIScene delegates and soon to be legacy standard delegate behavior). |
|
I still don't understand why this is needed. Is this for HMR? Because it seems like we're replacing irreplaceable parts of the application. Like if you change the app bootstrap or things that touch delegates we shouldn't expect the HMR to kick in instead of a full restart. Extended classes are dynamic already, so even if we reevaluate modules they'd just create new native classes that would get picked up when a new view is created. What does this bring in? Like 2s when someone touches critical files? |
|
This is for seamless production OTA (over-the-air) updates (unrelated to hmr). |
|
For OTA this is even worse! Half of people's delegates will be gone by that point and callbacks likely will just leak all around from the previous isolate. I could understand this for dev, but for prod it's just asking for crashes and weird behavior that are pretty much unfixable. Things like sentry and firebase will likely not work properly due to trying to initialize it multiple times for example |
|
This has already been in production for 6 months and proven stable, it's very helpful. I can show you offline how it works - will share details later this week. |
2 participants
Helpful for programmatic reset of JS isolate for clean restart of JS application as well as OTA (over-the-air) updates without restarting the entire app process.
NativeScript/NativeScript#11261
Summary by CodeRabbit
New Features
NativeScriptRuntimeAPI.reloadApplication(baseDir?)with optional base directory support.reloadCountvalue to observe how many reloads occurred.Bug Fixes