Skip to content

Commit 80a1111

Browse files
sunnylqmclaude
sunnylqm
and
claude
authored
perf(harmony): run patch/cleanup off the UI thread via async NAPI (HM-1) (#603)
* perf(harmony): run patch/cleanup off the UI thread via async NAPI (HM-1) The Pushy TurboModule runs on the UI thread, and applyPatchFromFileSource / cleanupOldEntries were synchronous NAPI calls, so hdiff patching and recursive cleanup froze the UI for hundreds of ms to seconds (Android already did this work on a background thread — this closes the platform gap). - C++: wrap both exports in napi_create_async_work returning a Promise. Args are parsed on the JS thread; the heavy hdiff/cleanup runs on a libuv worker thread; the Promise is settled back on the JS thread. - ArkTS bindings: applyPatchFromFileSource / cleanupOldEntries now return Promise<void>; all call sites in DownloadTask await them. - UpdateContext.cleanUp() keeps its void signature (all callers are fire-and-forget) but now launches the async cleanup with a .catch, so the cold-start syncStateWithBinaryVersion -> cleanUp path no longer blocks on synchronous disk I/O. Note: there is no HarmonyOS runner in CI and no local device, so this is not runtime-validated; needs an on-device pass of the full download -> patch -> switch -> restart -> markSuccess flow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(harmony): handle async-work create/queue failures (HM-1 review) napi_create_async_work / napi_queue_async_work return napi_status; if either fails the completion callback never runs, leaving the Promise pending forever and leaking the heap-allocated work data. Check both return values, and on failure delete the work (when created), reject the deferred, and free the data. Addresses CodeRabbit review on PR #603. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 7d959ad commit 80a1111

4 files changed

Lines changed: 166 additions & 32 deletions

File tree

‎harmony/pushy/src/main/cpp/pushy.cpp‎

Lines changed: 154 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -716,6 +716,47 @@ napi_value BuildCopyGroups(napi_env env, napi_callback_info info) {
716716
return NewCopyGroupArray(env, groups);
717717
}
718718

719+
// ---------------------------------------------------------------------------
720+
// Async work plumbing for the heavy patch operations.
721+
//
722+
// applyPatchFromFileSource and cleanupOldEntries run hdiff / recursive file IO
723+
// that can take hundreds of ms to seconds. The Pushy TurboModule executes on
724+
// the UI thread, so running these synchronously froze the UI. These are now
725+
// wrapped in napi_create_async_work: arguments are parsed on the JS thread, the
726+
// heavy work runs on a libuv worker thread, and the returned Promise is settled
727+
// back on the JS thread.
728+
// ---------------------------------------------------------------------------
729+
730+
// Reject an already-created deferred with an Error(message). Used when async
731+
// work fails to be created/queued, so the Promise never hangs pending.
732+
void RejectDeferredWithMessage(
733+
napi_env env,
734+
napi_deferred deferred,
735+
const char* message) {
736+
napi_value error = nullptr;
737+
napi_value message_value = nullptr;
738+
napi_create_string_utf8(env, message, NAPI_AUTO_LENGTH, &message_value);
739+
napi_create_error(env, nullptr, message_value, &error);
740+
napi_reject_deferred(env, deferred, error);
741+
}
742+
743+
struct ApplyPatchWork {
744+
napi_async_work work = nullptr;
745+
napi_deferred deferred = nullptr;
746+
pushy::patch::FileSourcePatchOptions options;
747+
pushy::patch::Status status{false, ""};
748+
};
749+
750+
struct CleanupWork {
751+
napi_async_work work = nullptr;
752+
napi_deferred deferred = nullptr;
753+
std::string root_dir;
754+
std::string keep_current;
755+
std::string keep_previous;
756+
int32_t max_age_days = 0;
757+
pushy::patch::Status status{false, ""};
758+
};
759+
719760
napi_value ApplyPatchFromFileSource(napi_env env, napi_callback_info info) {
720761
napi_value args[1] = {nullptr};
721762
size_t argc = 1;
@@ -755,26 +796,69 @@ napi_value ApplyPatchFromFileSource(napi_env env, napi_callback_info info) {
755796
return nullptr;
756797
}
757798

758-
pushy::patch::FileSourcePatchOptions options;
759-
options.manifest = BuildManifest(copy_froms, copy_tos, deletes);
760-
options.source_root = source_root;
761-
options.target_root = target_root;
762-
options.origin_bundle_path = origin_bundle_path;
763-
options.bundle_patch_path = bundle_patch_path;
764-
options.bundle_output_path = bundle_output_path;
765-
options.merge_source_subdir = merge_source_subdir;
766-
options.enable_merge = enable_merge;
799+
auto* work_data = new ApplyPatchWork();
800+
work_data->options.manifest = BuildManifest(copy_froms, copy_tos, deletes);
801+
work_data->options.source_root = source_root;
802+
work_data->options.target_root = target_root;
803+
work_data->options.origin_bundle_path = origin_bundle_path;
804+
work_data->options.bundle_patch_path = bundle_patch_path;
805+
work_data->options.bundle_output_path = bundle_output_path;
806+
work_data->options.merge_source_subdir = merge_source_subdir;
807+
work_data->options.enable_merge = enable_merge;
767808

768-
const pushy::patch::Status status =
769-
pushy::patch::ApplyPatchFromFileSource(options);
770-
if (!status.ok) {
771-
ThrowError(env, status.message);
809+
napi_value promise = nullptr;
810+
if (napi_create_promise(env, &work_data->deferred, &promise) != napi_ok) {
811+
delete work_data;
812+
ThrowError(env, "Unable to create promise");
772813
return nullptr;
773814
}
774815

775-
napi_value undefined_value = nullptr;
776-
napi_get_undefined(env, &undefined_value);
777-
return undefined_value;
816+
napi_value resource_name = nullptr;
817+
napi_create_string_utf8(
818+
env, "applyPatchFromFileSource", NAPI_AUTO_LENGTH, &resource_name);
819+
if (napi_create_async_work(
820+
env,
821+
nullptr,
822+
resource_name,
823+
[](napi_env, void* data) {
824+
auto* w = static_cast<ApplyPatchWork*>(data);
825+
w->status = pushy::patch::ApplyPatchFromFileSource(w->options);
826+
},
827+
[](napi_env cb_env, napi_status, void* data) {
828+
auto* w = static_cast<ApplyPatchWork*>(data);
829+
if (w->status.ok) {
830+
napi_value undefined_value = nullptr;
831+
napi_get_undefined(cb_env, &undefined_value);
832+
napi_resolve_deferred(cb_env, w->deferred, undefined_value);
833+
} else {
834+
napi_value error = nullptr;
835+
napi_value message = nullptr;
836+
napi_create_string_utf8(
837+
cb_env, w->status.message.c_str(), NAPI_AUTO_LENGTH, &message);
838+
napi_create_error(cb_env, nullptr, message, &error);
839+
napi_reject_deferred(cb_env, w->deferred, error);
840+
}
841+
napi_delete_async_work(cb_env, w->work);
842+
delete w;
843+
},
844+
work_data,
845+
&work_data->work) != napi_ok) {
846+
// Work was never created: settle the promise and free the data so it does
847+
// not leak / hang pending forever.
848+
RejectDeferredWithMessage(
849+
env, work_data->deferred, "Unable to create async work");
850+
delete work_data;
851+
return promise;
852+
}
853+
if (napi_queue_async_work(env, work_data->work) != napi_ok) {
854+
// Queued failed: the complete callback will never run, so clean up here.
855+
napi_delete_async_work(env, work_data->work);
856+
RejectDeferredWithMessage(
857+
env, work_data->deferred, "Unable to queue async work");
858+
delete work_data;
859+
return promise;
860+
}
861+
return promise;
778862
}
779863

780864
napi_value CleanupOldEntries(napi_env env, napi_callback_info info) {
@@ -803,19 +887,63 @@ napi_value CleanupOldEntries(napi_env env, napi_callback_info info) {
803887
return nullptr;
804888
}
805889

806-
const pushy::patch::Status status = pushy::patch::CleanupOldEntries(
807-
root_dir,
808-
keep_current,
809-
keep_previous,
810-
max_age_days);
811-
if (!status.ok) {
812-
ThrowError(env, status.message);
890+
auto* work_data = new CleanupWork();
891+
work_data->root_dir = root_dir;
892+
work_data->keep_current = keep_current;
893+
work_data->keep_previous = keep_previous;
894+
work_data->max_age_days = max_age_days;
895+
896+
napi_value promise = nullptr;
897+
if (napi_create_promise(env, &work_data->deferred, &promise) != napi_ok) {
898+
delete work_data;
899+
ThrowError(env, "Unable to create promise");
813900
return nullptr;
814901
}
815902

816-
napi_value undefined_value = nullptr;
817-
napi_get_undefined(env, &undefined_value);
818-
return undefined_value;
903+
napi_value resource_name = nullptr;
904+
napi_create_string_utf8(
905+
env, "cleanupOldEntries", NAPI_AUTO_LENGTH, &resource_name);
906+
if (napi_create_async_work(
907+
env,
908+
nullptr,
909+
resource_name,
910+
[](napi_env, void* data) {
911+
auto* w = static_cast<CleanupWork*>(data);
912+
w->status = pushy::patch::CleanupOldEntries(
913+
w->root_dir, w->keep_current, w->keep_previous, w->max_age_days);
914+
},
915+
[](napi_env cb_env, napi_status, void* data) {
916+
auto* w = static_cast<CleanupWork*>(data);
917+
if (w->status.ok) {
918+
napi_value undefined_value = nullptr;
919+
napi_get_undefined(cb_env, &undefined_value);
920+
napi_resolve_deferred(cb_env, w->deferred, undefined_value);
921+
} else {
922+
napi_value error = nullptr;
923+
napi_value message = nullptr;
924+
napi_create_string_utf8(
925+
cb_env, w->status.message.c_str(), NAPI_AUTO_LENGTH, &message);
926+
napi_create_error(cb_env, nullptr, message, &error);
927+
napi_reject_deferred(cb_env, w->deferred, error);
928+
}
929+
napi_delete_async_work(cb_env, w->work);
930+
delete w;
931+
},
932+
work_data,
933+
&work_data->work) != napi_ok) {
934+
RejectDeferredWithMessage(
935+
env, work_data->deferred, "Unable to create async work");
936+
delete work_data;
937+
return promise;
938+
}
939+
if (napi_queue_async_work(env, work_data->work) != napi_ok) {
940+
napi_delete_async_work(env, work_data->work);
941+
RejectDeferredWithMessage(
942+
env, work_data->deferred, "Unable to queue async work");
943+
delete work_data;
944+
return promise;
945+
}
946+
return promise;
819947
}
820948

821949
bool ExportFunction(

‎harmony/pushy/src/main/ets/DownloadTask.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ export class DownloadTask {
233233
const originBundlePath = `${workingDirectory}/${TEMP_ORIGIN_BUNDLE_ENTRY}`;
234234
try {
235235
await this.writeFileContent(originBundlePath, originContent);
236-
NativePatchCore.applyPatchFromFileSource({
236+
await NativePatchCore.applyPatchFromFileSource({
237237
copyFroms: [],
238238
copyTos: [],
239239
deletes: [],
@@ -568,7 +568,7 @@ export class DownloadTask {
568568
manifestArrays.deletes,
569569
HARMONY_BUNDLE_PATCH_ENTRY,
570570
);
571-
NativePatchCore.applyPatchFromFileSource({
571+
await NativePatchCore.applyPatchFromFileSource({
572572
copyFroms: manifestArrays.copyFroms,
573573
copyTos: manifestArrays.copyTos,
574574
deletes: manifestArrays.deletes,
@@ -664,7 +664,7 @@ export class DownloadTask {
664664

665665
private async doCleanUp(params: DownloadTaskParams): Promise<void> {
666666
try {
667-
NativePatchCore.cleanupOldEntries(
667+
await NativePatchCore.cleanupOldEntries(
668668
params.unzipDirectory,
669669
params.hash || '',
670670
params.originHash || '',

‎harmony/pushy/src/main/ets/NativePatchCore.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,13 +75,13 @@ interface NativePatchCoreBindings {
7575
bundlePatchEntryName?: string,
7676
): ArchivePatchPlanResult;
7777
buildCopyGroups(copyFroms: string[], copyTos: string[]): CopyGroupResult[];
78-
applyPatchFromFileSource(options: FileSourcePatchRequest): void;
78+
applyPatchFromFileSource(options: FileSourcePatchRequest): Promise<void>;
7979
cleanupOldEntries(
8080
rootDir: string,
8181
keepCurrent: string,
8282
keepPrevious: string,
8383
maxAgeDays: number,
84-
): void;
84+
): Promise<void>;
8585
}
8686

8787
export default NativeUpdateCore as unknown as NativePatchCoreBindings;

‎harmony/pushy/src/main/ets/UpdateContext.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -520,12 +520,18 @@ export class UpdateContext {
520520

521521
public cleanUp(): void {
522522
const state = this.getStateSnapshot();
523+
// cleanupOldEntries now runs on a native worker thread (returns a Promise).
524+
// Cleanup is best-effort background maintenance and no caller depends on its
525+
// completion, so fire-and-forget it off the UI thread and just log failures
526+
// instead of blocking the state operation (or cold start) on disk I/O.
523527
NativePatchCore.cleanupOldEntries(
524528
this.rootDir,
525529
state.currentVersion || '',
526530
state.lastVersion || '',
527531
3,
528-
);
532+
).catch((error: Object) => {
533+
console.error('cleanupOldEntries failed:', error);
534+
});
529535
}
530536

531537
public getIsUsingBundleUrl(): boolean {

0 commit comments

Comments
 (0)