fix(harmony): cache buildTime and packageVersion to prevent thread-safety sync errors#590
fix(harmony): cache buildTime and packageVersion to prevent thread-safety sync errors#590sunnylqm wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds a new ChangesExpo RCTBridge Config Plugin
Harmony UpdateContext Caching and Core Log Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
app.plugin.jsOops! Something went wrong! :( ESLint: 8.57.1 Error: .eslintrc.js » harmony/pushy/src/main/ets/UpdateContext.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: .eslintrc.js » 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: 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 `@Example/expoUsePushy/app.json`:
- Around line 29-30: The Expo plugin entry is pointing to the wrong module, so
the bridging-header patch will not be loaded. Update the plugin reference in
app.json to use the new app.plugin.js added at the repo root, and verify the
plugin name/path matches the actual file exported for the Expo config plugin.
In `@harmony/pushy/src/main/ets/UpdateContext.ts`:
- Around line 29-30: The lazy caching in UpdateContext still lets the first
background-thread call reach the unsafe synchronous metadata reads, so warm the
cache on the UI thread before any JS background access or make the background
path avoid reading meta.json when the cache is empty. Update the
UpdateContext/getConstants flow and the getBuildTime()/getPackageVersion() logic
so PushyTurboModule.getConstants() never falls back to empty build-time values
or triggers syncStateWithBinaryVersion() with an uninitialized cache.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0a8cac8-b6b7-424e-8ae6-637a98a2b733
⛔ Files ignored due to path filters (1)
Example/expoUsePushy/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Example/expoUsePushy/app.jsonExample/expoUsePushy/package.jsonapp.plugin.jsharmony/pushy/src/main/ets/UpdateContext.tssrc/core.ts
💤 Files with no reviewable changes (1)
- src/core.ts
| "plugins": [ | ||
| "./plugins/with-rct-bridge-bridging-header", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Point the plugin entry at the file added in this PR.
app.plugin.js is added at the repo root, but Line 30 registers ./plugins/with-rct-bridge-bridging-header. Unless another file exists at that exact path, Expo will fail to resolve the plugin and the bridging-header patch never runs.
Suggested fix
"plugins": [
- "./plugins/with-rct-bridge-bridging-header",
+ "../../app.plugin.js",
"expo-font",
"expo-router",
"expo-web-browser"
]📝 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.
| "plugins": [ | |
| "./plugins/with-rct-bridge-bridging-header", | |
| "plugins": [ | |
| "../../app.plugin.js", |
🤖 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 `@Example/expoUsePushy/app.json` around lines 29 - 30, The Expo plugin entry is
pointing to the wrong module, so the bridging-header patch will not be loaded.
Update the plugin reference in app.json to use the new app.plugin.js added at
the repo root, and verify the plugin name/path matches the actual file exported
for the Expo config plugin.
| private static cachedPackageVersion: string = ''; | ||
| private static cachedBuildTime: string = ''; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
This still allows the first background-thread read to hit the unsafe sync APIs.
The cache is only populated lazily, so if the first UpdateContext/getConstants() call happens on the JS background thread, Lines 68-72 and especially Lines 84-91 still do the synchronous lookups before anything is cached. That means getBuildTime() can still throw, return '', and then PushyTurboModule.getConstants() will pass that empty build time into syncStateWithBinaryVersion(), preserving the same preference-clearing path described in the PR objective. This needs a guaranteed UI-thread warm-up step before any background access, or the background path must avoid touching meta.json when the cache is empty.
Also applies to: 65-78, 80-96
🤖 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 `@harmony/pushy/src/main/ets/UpdateContext.ts` around lines 29 - 30, The lazy
caching in UpdateContext still lets the first background-thread call reach the
unsafe synchronous metadata reads, so warm the cache on the UI thread before any
JS background access or make the background path avoid reading meta.json when
the cache is empty. Update the UpdateContext/getConstants flow and the
getBuildTime()/getPackageVersion() logic so PushyTurboModule.getConstants()
never falls back to empty build-time values or triggers
syncStateWithBinaryVersion() with an uninitialized cache.
1 participant
This PR fixes a background-thread resource-safety issue that causes the update Preferences to be wiped after a soft reload:
getBuildTime()which runsresourceManager.getRawFileContentSync('meta.json'). In HarmonyOS, synchronous resource manager APIs are not thread-safe and throw exceptions if called from non-UI threads. The exception is silently caught and returns"". Because this doesn't match the previously stored build time, the system assumes the binary changed and clears the entire update Preferences.packageVersionandbuildTimein static class variables during the first resolution (which always runs on the Main/UI thread in the bundle provider). Background threads then read from memory cache directly without calling the rawfile APIs.Unit tests pass successfully.
Summary by CodeRabbit
New Features
Bug Fixes
Chores