feat: cancel() + stable error codes (#366) - #370
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| - (void)cancel:(RCTPromiseResolveBlock)resolve | ||
| reject:(RCTPromiseRejectBlock)reject { | ||
| (void)reject; | ||
| self.cancelled = YES; | ||
| resolve(nil); | ||
| } |
There was a problem hiding this comment.
🔴 Cancelling a running zip/unzip does nothing on iOS
The cancel request is queued on the same single-file work lane as the zip/unzip itself (cancel: at ios/RNZipArchive.mm:93-98), so it only runs after the operation it is meant to stop has already finished, and the next operation immediately clears the flag again.
Impact: On iOS, cancelling never aborts anything — long zip/unzip operations run to completion and their promises resolve normally instead of rejecting as cancelled.
Serial methodQueue serializes cancel behind the in-flight operation
methodQueue (ios/RNZipArchive.mm:769-776) is a DISPATCH_QUEUE_SERIAL queue, and all TurboModule methods of this module — including cancel: — are invoked on it. unzipFile: / unzipSelectedEntries: / zipFolder: run synchronously on that queue, so a cancel() call issued while one of them is executing sits in the queue until the work returns. By then the operation has already checked self.cancelled (still NO) and resolved.
Worse, every entry point starts with [self beginOperation] which sets self.cancelled = NO (ios/RNZipArchive.mm:81-83, called at lines 104, 151, 260, 492, 557, 651, 680, 715), so the flag set by the late-running cancel: is cleared at the start of the next operation and never observed. The RNZipCancelDelegate and the if (self.cancelled) branches added to every operation are therefore unreachable in practice.
The README example (README.md:205-213) does exactly const p = unzip(...); cancel(); and documents that the promise rejects with ERR_CANCELLED — on iOS it will resolve instead.
A fix requires running cancel() off the serial work queue (e.g. give cancel its own dispatch/RCT_EXPORT_METHOD-style main-queue execution, or store the cancel flag in an atomic that is written from a different queue), and not resetting the flag from inside the same serialized work item that a pending cancel is racing.
Prompt for agents
On iOS, cancel() is implemented as a TurboModule promise method (cancel:resolve:reject: in ios/RNZipArchive.mm). All methods of RNZipArchive are dispatched on the module's own serial methodQueue (dispatch_queue_create(..., DISPATCH_QUEUE_SERIAL) in -methodQueue). Because the zip/unzip methods execute synchronously on that queue, a cancel() invoked while an operation is in flight cannot run until that operation completes, so the cancelled flag is never seen by the running work. In addition, every operation entry point calls [self beginOperation] which resets self.cancelled = NO, so even a cancel that landed earlier is cleared. The net effect is that cancellation (the RNZipCancelDelegate hook, the rejectIfCancelled checks, and the ERR_CANCELLED rejections) is dead code on iOS, contradicting README.md's documented cancel() behaviour. Consider making cancel execute off the work queue (e.g. by not routing it through methodQueue, or dispatching the flag write to another queue) and storing the flag in an atomic/os_unfair_lock-protected variable so it is visible from the worker; also reconsider resetting the flag inside the same serialized work item so a pending cancel is not swallowed.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed. Zip/unzip/listContents now run on a dedicated serial workQueue. cancel() still lands on the RCT method queue (or JS native thread), so it can set the atomic flag while work is in flight. SSZipArchive's unzip delegate and the selective-extract I/O loop observe that flag.
| private void beginOperation() { | ||
| cancelled.set(false); | ||
| } |
There was a problem hiding this comment.
🟡 Cancelling right after starting an operation is silently ignored on Android
The cancel flag is cleared at the moment the queued zip/unzip work actually starts (beginOperation() at android/src/main/java/com/rnziparchive/RNZipArchiveModule.java:81-83), which happens after the cancel request may already have arrived, so the cancellation is discarded and the operation runs to completion.
Impact: Cancelling immediately after starting an operation — the pattern shown in the docs — intermittently does nothing and the operation completes as if never cancelled.
Race between cancel() on the native-modules thread and beginOperation() on the worker thread
cancel(Promise) (android/src/main/java/com/rnziparchive/RNZipArchiveModule.java:102-106) runs on the React native-modules thread and sets cancelled = true. All work runs on the single-thread executor; each submitted task begins with beginOperation() which does cancelled.set(false) (e.g. unzip at RNZipArchiveModule.java:179-180, unzipWithPassword at 132-133, processZip at 602-603).
If cancel() is processed before the executor thread has picked up the just-submitted task (the common case for const p = unzip(...); cancel(); documented in README.md:205-213), the flag is reset to false and none of the rejectIfCancelled checks fire. The behaviour is timing dependent.
A more robust approach is to reset the cancel state when the operation is enqueued (on the calling thread, before executor.submit) rather than inside the task body, or to use a per-operation token/generation counter that cancel() targets.
Prompt for agents
In android/src/main/java/com/rnziparchive/RNZipArchiveModule.java, beginOperation() (which clears the shared AtomicBoolean `cancelled`) is invoked as the first statement inside every executor task (unzip, unzipWithPassword, listContents, extractSelectedEntries, processZip, unzipAssets, isPasswordProtected, getUncompressedSize). cancel(Promise) sets the flag from the React native-modules thread. When JS starts an operation and immediately calls cancel() (the pattern documented in README.md), the cancel can win the race and be wiped by beginOperation() when the worker thread finally starts the task, so the operation is never aborted. Consider resetting the cancel state synchronously on the calling thread before executor.submit(...), or replacing the single boolean with a per-operation token/generation so cancel() unambiguously targets the operation the caller just started.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed. beginOperation() now runs in submitWork() on the calling thread before executor.submit, so a cancel() that arrives after the JS call returns but before the worker starts is no longer cleared.
| if (password.length > 0) { | ||
| ret = unzOpenCurrentFilePassword(zip, [password cStringUsingEncoding:NSUTF8StringEncoding]); | ||
| } else { | ||
| ret = unzOpenCurrentFile(zip); | ||
| } | ||
| if (ret != UNZ_OK) { | ||
| success = NO; | ||
| extractError = [NSError errorWithDomain:@"RNZipArchive" | ||
| code:-1 | ||
| userInfo:@{NSLocalizedDescriptionKey: @"failed to open file in zip archive"}]; | ||
| break; | ||
| } |
There was a problem hiding this comment.
🟡 Wrong password during selective extract reports a generic failure instead of a password error on iOS
When a selected entry cannot be opened, the failure is always reported as a generic unzip problem (extractError message "failed to open file in zip archive" at ios/RNZipArchive.mm:420-426) even when the cause is an incorrect password, so callers see a different error code than the one documented and than Android returns.
Impact: Apps that branch on the password-error code cannot detect a wrong password when extracting selected entries on iOS.
Code mapping never yields ERR_WRONG_PASSWORD for the open failure
In unzipSelectedEntries (ios/RNZipArchive.mm:415-426), unzOpenCurrentFilePassword returns a non-UNZ_OK status when the supplied password fails minizip's verification, and the error message is hard-coded to @"failed to open file in zip archive". The final mapping at ios/RNZipArchive.mm:476-483 only picks kZipErrWrongPassword when the message contains "password", so this path always rejects with ERR_UNZIP.
Android's selective path rejects with ERR_WRONG_PASSWORD for the same condition via ZipErrorCodes.mapException (android/src/main/java/com/rnziparchive/ZipErrorCodes.java:29-45), and README.md:224 documents ERR_WRONG_PASSWORD for a failed password decrypt. Related: this iOS path also never checks whether the archive is encrypted at all, so it cannot return ERR_NOT_PASSWORD_PROTECTED the way Android does (RNZipArchiveModule.java:304-311).
Prompt for agents
ios/RNZipArchive.mm unzipSelectedEntries: when unzOpenCurrentFilePassword / unzOpenCurrentFile fails, the error message is hard-coded to "failed to open file in zip archive", and the final error-code mapping (which only looks for the substring "password") therefore always produces ERR_UNZIP. For a wrong password minizip returns a password-specific status, so this should be distinguished and rejected as ERR_WRONG_PASSWORD to match the Android implementation and the documented error table in README.md. Also consider detecting the case where a password was supplied but the archive is not encrypted, so iOS can reject with ERR_NOT_PASSWORD_PROTECTED like Android does.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed. Selective extract maps unzOpenCurrentFilePassword / read failures to ERR_WRONG_PASSWORD when a password was supplied, and rejects ERR_NOT_PASSWORD_PROTECTED when unzipWithPassword is used on a plain archive. CRC failure after a passworded open is also mapped to ERR_WRONG_PASSWORD.
| s.pod_target_xcconfig = { | ||
| 'HEADER_SEARCH_PATHS' => '$(inherited) "$(PODS_ROOT)/SSZipArchive/SSZipArchive/minizip"' | ||
| } |
There was a problem hiding this comment.
🔍 New private minizip header dependency via pod_target_xcconfig
#import "mz_compat.h" in ios/RNZipArchive.mm:10 relies on the added HEADER_SEARCH_PATHS entry pointing at $(PODS_ROOT)/SSZipArchive/SSZipArchive/minizip, i.e. a non-public header path inside the SSZipArchive pod. This is fragile across SSZipArchive versions and breaks under use_frameworks! where headers are consumed from framework umbrella paths rather than PODS_ROOT. Worth verifying an install with use_frameworks! :linkage => :static and with the pinned ~>2.5.5 upper bound.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Keeping this as-is. SSZipArchive 2.5.5 does not publish minizip headers (mz_compat.h / unzGetCurrentFileInfo64) as public pod headers, and listContents / selective extract need those APIs. HEADER_SEARCH_PATHS is the workaround for this dependency version; both playground iOS builds already compile with it. #371 adds __has_include fallbacks (<SSZipArchive/minizip/mz_compat.h>, "mz_compat.h", "unzip.h") so a future SSZipArchive layout does not hard-fail.
| } else if ("STANDARD".equals(encryptionMethod)) { | ||
| parameters.setEncryptionMethod(EncryptionMethod.ZIP_STANDARD_VARIANT_STRONG); | ||
| // ZipCrypto (ZIP_STANDARD). ZIP_STANDARD_VARIANT_STRONG is write-only in zip4j | ||
| // and fails extract with "encryption method is not supported". | ||
| parameters.setEncryptionMethod(EncryptionMethod.ZIP_STANDARD); |
There was a problem hiding this comment.
🔍 Encryption method change for STANDARD affects newly created archives
Switching STANDARD from ZIP_STANDARD_VARIANT_STRONG to ZIP_STANDARD changes the on-disk format of password-protected archives created on Android. The inline comment explains this fixes extraction, but note it is a behavioural change bundled into a PR whose stated scope is cancel()/error codes, and it is not mentioned in the CHANGELOG entries added here — archives written by earlier versions with the variant-strong method remain unreadable.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Documented in CHANGELOG 9.1.0 and README. The on-disk change to ZipCrypto (ZIP_STANDARD) is intentional: ZIP_STANDARD_VARIANT_STRONG is write-only in zip4j and produced archives that this library could not extract. 'STANDARD' is traditional ZIP encryption, not PKWARE Strong Encryption.
Rebased onto master after #369 was squash-merged so this PR no longer conflicts with the rewritten listContents history. Co-authored-by: Perry <plrthink@gmail.com>
bdf8040 to
64090f1
Compare
Make iOS cancel() interrupt in-flight work by running zip/unzip on a background queue instead of the serial method queue. Reset Android's cancel flag when enqueueing work so a cancel right after start is not discarded. Selective extract now checks fwrite and CRC, maps password failures to ERR_WRONG_PASSWORD, and listContents uses 64-bit zip info. Document Android STANDARD encryption as ZipCrypto. Co-authored-by: Perry <plrthink@gmail.com>
2 participants
Summary
Implements #366 (P0):
cancel()and stable cross-platform error codes.Rebased onto
masterafter #369 was squash-merged so this PR is conflict-free.APIs
cancel()— best-effort abort of in-flight zip/unzipErrorCodesmap (ERR_CANCELLED,ERR_FILE_NOT_FOUND,ERR_WRONG_PASSWORD,ERR_UNSAFE_PATH, …)Review follow-up
Addressed Devin review comments on this PR and on merged #369:
Fixed
cancel()now interrupts in-flight work (zip/unzip run on a background serial queue, not the RCT method queue)beginOperation()runs when work is enqueued, socancel()immediately after start is not discardedfwriteandunzCloseCurrentFileCRCERR_WRONG_PASSWORD/ERR_NOT_PASSWORD_PROTECTEDlistContentsusesunzGetCurrentFileInfo64for entries ≥ 4 GiB'STANDARD'as ZipCrypto (ZIP_STANDARD) in CHANGELOG + READMEPushed back
HEADER_SEARCH_PATHSfor SSZipArchive minizip — required formz_compat.h/ 64-bit unzip APIs on SSZipArchive 2.5.5, which does not publish those headers. CI already builds both playgrounds with this podspec; fix(ios): zip interoperability for server-side unzippers (#367) #371 adds__has_includefallbacks.Stack
Merge order: #369 (done) → this PR → #371 → #372
Prefer a merge commit (not squash) for the remaining stacked PRs so the next ones stay conflict-free.