From 6570733e7e34cef85642c9aea48aef2181c7bb42 Mon Sep 17 00:00:00 2001 From: sunnylqm Date: Sat, 4 Jul 2026 21:49:59 +0800 Subject: [PATCH 1/2] perf(harmony): run patch/cleanup off the UI thread via async NAPI (HM-1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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; 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 --- harmony/pushy/src/main/cpp/pushy.cpp | 156 ++++++++++++++---- harmony/pushy/src/main/ets/DownloadTask.ts | 6 +- harmony/pushy/src/main/ets/NativePatchCore.ts | 4 +- harmony/pushy/src/main/ets/UpdateContext.ts | 8 +- 4 files changed, 135 insertions(+), 39 deletions(-) diff --git a/harmony/pushy/src/main/cpp/pushy.cpp b/harmony/pushy/src/main/cpp/pushy.cpp index 025b9414..ba3dce63 100644 --- a/harmony/pushy/src/main/cpp/pushy.cpp +++ b/harmony/pushy/src/main/cpp/pushy.cpp @@ -716,6 +716,34 @@ napi_value BuildCopyGroups(napi_env env, napi_callback_info info) { return NewCopyGroupArray(env, groups); } +// --------------------------------------------------------------------------- +// Async work plumbing for the heavy patch operations. +// +// applyPatchFromFileSource and cleanupOldEntries run hdiff / recursive file IO +// that can take hundreds of ms to seconds. The Pushy TurboModule executes on +// the UI thread, so running these synchronously froze the UI. These are now +// wrapped in napi_create_async_work: arguments are parsed on the JS thread, the +// heavy work runs on a libuv worker thread, and the returned Promise is settled +// back on the JS thread. +// --------------------------------------------------------------------------- + +struct ApplyPatchWork { + napi_async_work work = nullptr; + napi_deferred deferred = nullptr; + pushy::patch::FileSourcePatchOptions options; + pushy::patch::Status status{false, ""}; +}; + +struct CleanupWork { + napi_async_work work = nullptr; + napi_deferred deferred = nullptr; + std::string root_dir; + std::string keep_current; + std::string keep_previous; + int32_t max_age_days = 0; + pushy::patch::Status status{false, ""}; +}; + napi_value ApplyPatchFromFileSource(napi_env env, napi_callback_info info) { napi_value args[1] = {nullptr}; size_t argc = 1; @@ -755,26 +783,55 @@ napi_value ApplyPatchFromFileSource(napi_env env, napi_callback_info info) { return nullptr; } - pushy::patch::FileSourcePatchOptions options; - options.manifest = BuildManifest(copy_froms, copy_tos, deletes); - options.source_root = source_root; - options.target_root = target_root; - options.origin_bundle_path = origin_bundle_path; - options.bundle_patch_path = bundle_patch_path; - options.bundle_output_path = bundle_output_path; - options.merge_source_subdir = merge_source_subdir; - options.enable_merge = enable_merge; - - const pushy::patch::Status status = - pushy::patch::ApplyPatchFromFileSource(options); - if (!status.ok) { - ThrowError(env, status.message); - return nullptr; - } - - napi_value undefined_value = nullptr; - napi_get_undefined(env, &undefined_value); - return undefined_value; + auto* work_data = new ApplyPatchWork(); + work_data->options.manifest = BuildManifest(copy_froms, copy_tos, deletes); + work_data->options.source_root = source_root; + work_data->options.target_root = target_root; + work_data->options.origin_bundle_path = origin_bundle_path; + work_data->options.bundle_patch_path = bundle_patch_path; + work_data->options.bundle_output_path = bundle_output_path; + work_data->options.merge_source_subdir = merge_source_subdir; + work_data->options.enable_merge = enable_merge; + + napi_value promise = nullptr; + if (napi_create_promise(env, &work_data->deferred, &promise) != napi_ok) { + delete work_data; + ThrowError(env, "Unable to create promise"); + return nullptr; + } + + napi_value resource_name = nullptr; + napi_create_string_utf8( + env, "applyPatchFromFileSource", NAPI_AUTO_LENGTH, &resource_name); + napi_create_async_work( + env, + nullptr, + resource_name, + [](napi_env, void* data) { + auto* w = static_cast(data); + w->status = pushy::patch::ApplyPatchFromFileSource(w->options); + }, + [](napi_env cb_env, napi_status, void* data) { + auto* w = static_cast(data); + if (w->status.ok) { + napi_value undefined_value = nullptr; + napi_get_undefined(cb_env, &undefined_value); + napi_resolve_deferred(cb_env, w->deferred, undefined_value); + } else { + napi_value error = nullptr; + napi_value message = nullptr; + napi_create_string_utf8( + cb_env, w->status.message.c_str(), NAPI_AUTO_LENGTH, &message); + napi_create_error(cb_env, nullptr, message, &error); + napi_reject_deferred(cb_env, w->deferred, error); + } + napi_delete_async_work(cb_env, w->work); + delete w; + }, + work_data, + &work_data->work); + napi_queue_async_work(env, work_data->work); + return promise; } napi_value CleanupOldEntries(napi_env env, napi_callback_info info) { @@ -803,19 +860,52 @@ napi_value CleanupOldEntries(napi_env env, napi_callback_info info) { return nullptr; } - const pushy::patch::Status status = pushy::patch::CleanupOldEntries( - root_dir, - keep_current, - keep_previous, - max_age_days); - if (!status.ok) { - ThrowError(env, status.message); - return nullptr; - } - - napi_value undefined_value = nullptr; - napi_get_undefined(env, &undefined_value); - return undefined_value; + auto* work_data = new CleanupWork(); + work_data->root_dir = root_dir; + work_data->keep_current = keep_current; + work_data->keep_previous = keep_previous; + work_data->max_age_days = max_age_days; + + napi_value promise = nullptr; + if (napi_create_promise(env, &work_data->deferred, &promise) != napi_ok) { + delete work_data; + ThrowError(env, "Unable to create promise"); + return nullptr; + } + + napi_value resource_name = nullptr; + napi_create_string_utf8( + env, "cleanupOldEntries", NAPI_AUTO_LENGTH, &resource_name); + napi_create_async_work( + env, + nullptr, + resource_name, + [](napi_env, void* data) { + auto* w = static_cast(data); + w->status = pushy::patch::CleanupOldEntries( + w->root_dir, w->keep_current, w->keep_previous, w->max_age_days); + }, + [](napi_env cb_env, napi_status, void* data) { + auto* w = static_cast(data); + if (w->status.ok) { + napi_value undefined_value = nullptr; + napi_get_undefined(cb_env, &undefined_value); + napi_resolve_deferred(cb_env, w->deferred, undefined_value); + } else { + napi_value error = nullptr; + napi_value message = nullptr; + napi_create_string_utf8( + cb_env, w->status.message.c_str(), NAPI_AUTO_LENGTH, &message); + napi_create_error(cb_env, nullptr, message, &error); + napi_reject_deferred(cb_env, w->deferred, error); + } + napi_delete_async_work(cb_env, w->work); + delete w; + }, + work_data, + &work_data->work); + napi_queue_async_work(env, work_data->work); + return promise; } bool ExportFunction( diff --git a/harmony/pushy/src/main/ets/DownloadTask.ts b/harmony/pushy/src/main/ets/DownloadTask.ts index 46c6de68..28715113 100644 --- a/harmony/pushy/src/main/ets/DownloadTask.ts +++ b/harmony/pushy/src/main/ets/DownloadTask.ts @@ -233,7 +233,7 @@ export class DownloadTask { const originBundlePath = `${workingDirectory}/${TEMP_ORIGIN_BUNDLE_ENTRY}`; try { await this.writeFileContent(originBundlePath, originContent); - NativePatchCore.applyPatchFromFileSource({ + await NativePatchCore.applyPatchFromFileSource({ copyFroms: [], copyTos: [], deletes: [], @@ -568,7 +568,7 @@ export class DownloadTask { manifestArrays.deletes, HARMONY_BUNDLE_PATCH_ENTRY, ); - NativePatchCore.applyPatchFromFileSource({ + await NativePatchCore.applyPatchFromFileSource({ copyFroms: manifestArrays.copyFroms, copyTos: manifestArrays.copyTos, deletes: manifestArrays.deletes, @@ -664,7 +664,7 @@ export class DownloadTask { private async doCleanUp(params: DownloadTaskParams): Promise { try { - NativePatchCore.cleanupOldEntries( + await NativePatchCore.cleanupOldEntries( params.unzipDirectory, params.hash || '', params.originHash || '', diff --git a/harmony/pushy/src/main/ets/NativePatchCore.ts b/harmony/pushy/src/main/ets/NativePatchCore.ts index ba680275..7f3c6bdc 100644 --- a/harmony/pushy/src/main/ets/NativePatchCore.ts +++ b/harmony/pushy/src/main/ets/NativePatchCore.ts @@ -75,13 +75,13 @@ interface NativePatchCoreBindings { bundlePatchEntryName?: string, ): ArchivePatchPlanResult; buildCopyGroups(copyFroms: string[], copyTos: string[]): CopyGroupResult[]; - applyPatchFromFileSource(options: FileSourcePatchRequest): void; + applyPatchFromFileSource(options: FileSourcePatchRequest): Promise; cleanupOldEntries( rootDir: string, keepCurrent: string, keepPrevious: string, maxAgeDays: number, - ): void; + ): Promise; } export default NativeUpdateCore as unknown as NativePatchCoreBindings; diff --git a/harmony/pushy/src/main/ets/UpdateContext.ts b/harmony/pushy/src/main/ets/UpdateContext.ts index 30432ef1..f026a4bf 100644 --- a/harmony/pushy/src/main/ets/UpdateContext.ts +++ b/harmony/pushy/src/main/ets/UpdateContext.ts @@ -520,12 +520,18 @@ export class UpdateContext { public cleanUp(): void { const state = this.getStateSnapshot(); + // cleanupOldEntries now runs on a native worker thread (returns a Promise). + // Cleanup is best-effort background maintenance and no caller depends on its + // completion, so fire-and-forget it off the UI thread and just log failures + // instead of blocking the state operation (or cold start) on disk I/O. NativePatchCore.cleanupOldEntries( this.rootDir, state.currentVersion || '', state.lastVersion || '', 3, - ); + ).catch((error: Object) => { + console.error('cleanupOldEntries failed:', error); + }); } public getIsUsingBundleUrl(): boolean { From 827b6fd154a67ce70439c15cc7527bd63a2b3dd0 Mon Sep 17 00:00:00 2001 From: sunnylqm Date: Sat, 4 Jul 2026 22:57:37 +0800 Subject: [PATCH 2/2] 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 --- harmony/pushy/src/main/cpp/pushy.cpp | 152 +++++++++++++++++---------- 1 file changed, 95 insertions(+), 57 deletions(-) diff --git a/harmony/pushy/src/main/cpp/pushy.cpp b/harmony/pushy/src/main/cpp/pushy.cpp index ba3dce63..65e69e72 100644 --- a/harmony/pushy/src/main/cpp/pushy.cpp +++ b/harmony/pushy/src/main/cpp/pushy.cpp @@ -727,6 +727,19 @@ napi_value BuildCopyGroups(napi_env env, napi_callback_info info) { // back on the JS thread. // --------------------------------------------------------------------------- +// Reject an already-created deferred with an Error(message). Used when async +// work fails to be created/queued, so the Promise never hangs pending. +void RejectDeferredWithMessage( + napi_env env, + napi_deferred deferred, + const char* message) { + napi_value error = nullptr; + napi_value message_value = nullptr; + napi_create_string_utf8(env, message, NAPI_AUTO_LENGTH, &message_value); + napi_create_error(env, nullptr, message_value, &error); + napi_reject_deferred(env, deferred, error); +} + struct ApplyPatchWork { napi_async_work work = nullptr; napi_deferred deferred = nullptr; @@ -803,34 +816,48 @@ napi_value ApplyPatchFromFileSource(napi_env env, napi_callback_info info) { napi_value resource_name = nullptr; napi_create_string_utf8( env, "applyPatchFromFileSource", NAPI_AUTO_LENGTH, &resource_name); - napi_create_async_work( - env, - nullptr, - resource_name, - [](napi_env, void* data) { - auto* w = static_cast(data); - w->status = pushy::patch::ApplyPatchFromFileSource(w->options); - }, - [](napi_env cb_env, napi_status, void* data) { - auto* w = static_cast(data); - if (w->status.ok) { - napi_value undefined_value = nullptr; - napi_get_undefined(cb_env, &undefined_value); - napi_resolve_deferred(cb_env, w->deferred, undefined_value); - } else { - napi_value error = nullptr; - napi_value message = nullptr; - napi_create_string_utf8( - cb_env, w->status.message.c_str(), NAPI_AUTO_LENGTH, &message); - napi_create_error(cb_env, nullptr, message, &error); - napi_reject_deferred(cb_env, w->deferred, error); - } - napi_delete_async_work(cb_env, w->work); - delete w; - }, - work_data, - &work_data->work); - napi_queue_async_work(env, work_data->work); + if (napi_create_async_work( + env, + nullptr, + resource_name, + [](napi_env, void* data) { + auto* w = static_cast(data); + w->status = pushy::patch::ApplyPatchFromFileSource(w->options); + }, + [](napi_env cb_env, napi_status, void* data) { + auto* w = static_cast(data); + if (w->status.ok) { + napi_value undefined_value = nullptr; + napi_get_undefined(cb_env, &undefined_value); + napi_resolve_deferred(cb_env, w->deferred, undefined_value); + } else { + napi_value error = nullptr; + napi_value message = nullptr; + napi_create_string_utf8( + cb_env, w->status.message.c_str(), NAPI_AUTO_LENGTH, &message); + napi_create_error(cb_env, nullptr, message, &error); + napi_reject_deferred(cb_env, w->deferred, error); + } + napi_delete_async_work(cb_env, w->work); + delete w; + }, + work_data, + &work_data->work) != napi_ok) { + // Work was never created: settle the promise and free the data so it does + // not leak / hang pending forever. + RejectDeferredWithMessage( + env, work_data->deferred, "Unable to create async work"); + delete work_data; + return promise; + } + if (napi_queue_async_work(env, work_data->work) != napi_ok) { + // Queued failed: the complete callback will never run, so clean up here. + napi_delete_async_work(env, work_data->work); + RejectDeferredWithMessage( + env, work_data->deferred, "Unable to queue async work"); + delete work_data; + return promise; + } return promise; } @@ -876,35 +903,46 @@ napi_value CleanupOldEntries(napi_env env, napi_callback_info info) { napi_value resource_name = nullptr; napi_create_string_utf8( env, "cleanupOldEntries", NAPI_AUTO_LENGTH, &resource_name); - napi_create_async_work( - env, - nullptr, - resource_name, - [](napi_env, void* data) { - auto* w = static_cast(data); - w->status = pushy::patch::CleanupOldEntries( - w->root_dir, w->keep_current, w->keep_previous, w->max_age_days); - }, - [](napi_env cb_env, napi_status, void* data) { - auto* w = static_cast(data); - if (w->status.ok) { - napi_value undefined_value = nullptr; - napi_get_undefined(cb_env, &undefined_value); - napi_resolve_deferred(cb_env, w->deferred, undefined_value); - } else { - napi_value error = nullptr; - napi_value message = nullptr; - napi_create_string_utf8( - cb_env, w->status.message.c_str(), NAPI_AUTO_LENGTH, &message); - napi_create_error(cb_env, nullptr, message, &error); - napi_reject_deferred(cb_env, w->deferred, error); - } - napi_delete_async_work(cb_env, w->work); - delete w; - }, - work_data, - &work_data->work); - napi_queue_async_work(env, work_data->work); + if (napi_create_async_work( + env, + nullptr, + resource_name, + [](napi_env, void* data) { + auto* w = static_cast(data); + w->status = pushy::patch::CleanupOldEntries( + w->root_dir, w->keep_current, w->keep_previous, w->max_age_days); + }, + [](napi_env cb_env, napi_status, void* data) { + auto* w = static_cast(data); + if (w->status.ok) { + napi_value undefined_value = nullptr; + napi_get_undefined(cb_env, &undefined_value); + napi_resolve_deferred(cb_env, w->deferred, undefined_value); + } else { + napi_value error = nullptr; + napi_value message = nullptr; + napi_create_string_utf8( + cb_env, w->status.message.c_str(), NAPI_AUTO_LENGTH, &message); + napi_create_error(cb_env, nullptr, message, &error); + napi_reject_deferred(cb_env, w->deferred, error); + } + napi_delete_async_work(cb_env, w->work); + delete w; + }, + work_data, + &work_data->work) != napi_ok) { + RejectDeferredWithMessage( + env, work_data->deferred, "Unable to create async work"); + delete work_data; + return promise; + } + if (napi_queue_async_work(env, work_data->work) != napi_ok) { + napi_delete_async_work(env, work_data->work); + RejectDeferredWithMessage( + env, work_data->deferred, "Unable to queue async work"); + delete work_data; + return promise; + } return promise; }