Repository navigation
Conversation
StorageUploadTask and StorageDownloadTask await `fetcher.beginFetch()`. `cancel()`, and `pause()` on a download, stop the fetcher, and GTMSessionFetcher then releases its completion handler without calling it. The awaiting continuation never resumes, so the suspended Task keeps the storage task, and any data being uploaded, alive forever. A paused download leaks even after it's resumed and finishes. Start the fetch with the completion handler form of `beginFetch` instead. The closure keeps the task alive while the fetch runs, as the suspended Task did, and is released when the fetch is stopped, so the task can be deallocated.
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a memory leak in StorageUploadTask and StorageDownloadTask where tasks and their associated data were not released after being cancelled or paused. The fix refactors the fetch mechanism to use completion handlers instead of try await blocks, and adds unit tests to verify the task release behavior. Feedback on the changes points out a redundant lock check in StorageDownloadTask.swift that can be removed to simplify the code and improve efficiency.
Remove the separate locked `state == .cancelled` check at the start of `fetchDidSucceed(with:)`. The locked block that follows already returns early for a cancelled task, leaving both snapshots nil so nothing fires, so the extra lock acquisition added nothing. Addresses review feedback.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a memory leak in StorageUploadTask and StorageDownloadTask where tasks and uploaded data were not released after being cancelled or paused. The implementation refactors both tasks to use beginFetch with a completion handler instead of try await, preventing awaiting tasks from being suspended indefinitely when a fetcher is stopped. Additionally, a new unit test suite StorageTaskTests has been introduced to verify that these tasks are correctly released under various conditions. I have no feedback to provide as there are no review comments.
Problem
StorageUploadTaskandStorageDownloadTaskwait ontry await fetcher.beginFetch().cancel(),and
pause()on a download, callstopFetching(). Storage doesn't setstopFetchingTriggersCompletionHandler, sostopFetching()releases the fetcher's completionhandler without calling it. The awaiting continuation never resumes, and the suspended internal
Taskkeeps the Storage task alive forever:putData.Taskfor the original request stays suspended.
Change
beginFetch, in a smallsynchronous
startFetch(with:)helper. Calling that form directly from the async code triggers a"consider using asynchronous alternative function" warning.
fetchDidSucceed(with:)andfetchDidFail(with:)helpers. It's unchanged apart from dropping a redundant cancelled check atthe start of the download success path; the locked block after it already makes that check.
Taskdid, soa transfer still finishes when the caller doesn't hold the task. When
stopFetching()releasesthe callbacks, the closure goes with them and the task can be deallocated.
nilresponse body is still delivered as emptyData, as the async API bridged it. A new testcovers this.
progress callbacks already run) instead of the concurrency global executor. It only updates state
under the lock and dispatches handlers to
Storage.callbackQueue, as before.Setting
stopFetchingTriggersCompletionHandlerisn't a drop-in alternative. Upload fetchers copy itto their chunk fetchers, and
pauseFetching()stops the chunk fetcher, so pausing an upload woulddeliver a final error and break resume.
Testing
Added
StorageTaskTests:cancel(). These use a test block that neverresponds, so the request is in flight when it's cancelled.
pause(),resume(), and success.getDatastill returns emptyDatafor an empty response.The task was not released. The three regression tests pass.xcodebuild -scheme FirebaseStorageUnit test -destination 'platform=macOS':169 tests, 0 failures.
I also ran a temporary test file (not included) against the Storage emulator:
pause/resume, with contents checked.
On
main, the cancelled tasks and the resumed download were never released. With the change, allof them are released, and the transfers still succeed with the same contents.
Related PRs
often.