Download support #33

matejmolnar · 2023-03-07T21:35:59Z

A rough implementation of a working DownloadAPIManager with an example scene.

Separate manager to deal with download tasks DownloadAPIManager
It's now possible to use background urlSession configuration specific for downloading files
Retry handling moved to a protocol Retryable to avoid duplicate code
Download simulation UI to test download tasks with the option to run/suspend/cancel

cejanen

Hola, 👏
thank you, finally checked the PR, I would say the current approach is feasible and nice. I found minor changes we can improve

NetworkingSampleApp/NetworkingSampleApp/API/Routers/SampleDownloadRouter.swift

NetworkingSampleApp/NetworkingSampleApp/ContentView.swift

NetworkingSampleApp/NetworkingSampleApp/Scenes/Download/DownloadRow.swift

NetworkingSampleApp/NetworkingSampleApp/Scenes/Download/DownloadRowViewModel.swift

NetworkingSampleApp/NetworkingSampleApp/Scenes/Download/DownloadsViewModel.swift

Sources/Networking/Core/DownloadAPIManager.swift

brenovaladao

🪙

NetworkingSampleApp/NetworkingSampleApp/Scenes/Download/DownloadProgressViewModel.swift

brenovaladao · 2023-05-24T13:06:54Z

NetworkingSampleApp/NetworkingSampleApp/Scenes/Download/DownloadProgressView.swift

+        .onAppear {
+            viewModel.onAppear()
+        }


Suggested change

.onAppear {

viewModel.onAppear()

}

.task {

await viewModel.onAppear()

}

👍 thinking also about better name for viewModel function, maybe startDownloadTasks maybe better? Or any other suggestions?

It's not starting the download, so I'd suggest something like startObservingDownloadProgress.

brenovaladao · 2023-05-24T13:07:16Z

NetworkingSampleApp/NetworkingSampleApp/Scenes/Download/DownloadsViewModel.swift

+    func onAppear() {
+        Task {


method not used and not necessary

Sources/Networking/Core/DownloadAPIManager.swift

tonyskansf

👋

The sample app crashes on both the simulator and real device when multiple downloads are ongoing (at 5 downloads). Reproducible on every try.

See crash logs.

Sources/Networking/Misc/URLSessionTask+asyncResponse.swift

NetworkingSampleApp/NetworkingSampleApp/Scenes/Download/DownloadProgressViewModel.swift

tonyskansf · 2023-05-24T13:20:42Z

Sources/Networking/Core/DownloadAPIManager.swift

+    public func urlSession(_: URLSession, downloadTask: URLSessionDownloadTask, didWriteData _: Int64, totalBytesWritten: Int64, totalBytesExpectedToWrite: Int64) {
+        downloadStateDict[downloadTask]?.downloadedBytes = totalBytesWritten
+        downloadStateDict[downloadTask]?.totalBytes = totalBytesExpectedToWrite
+    }


Could this be the place where (accessing the downloadStateDict) it's not thread-safe and perhaps causing the crash? Maybe using an actor would be safer.

👍 similar we have for running calls and retry mechanism

agreed, all props which are being mutated should be thread safe

Sources/Networking/Core/DownloadAPIManager.swift

Sources/Networking/Misc/URLSessionTask+asyncResponse.swift

NetworkingSampleApp/NetworkingSampleApp/Scenes/Download/DownloadProgressView.swift

cejanen

My pieces also added
Thanks guys for great comments

NetworkingSampleApp/NetworkingSampleApp/Extensions/DownloadAPIManager+instance.swift

NetworkingSampleApp/NetworkingSampleApp/Scenes/Download/DownloadProgressViewModel.swift

cejanen · 2023-05-24T17:02:17Z

NetworkingSampleApp/NetworkingSampleApp/Scenes/Download/DownloadProgressView.swift

+        .onAppear {
+            viewModel.onAppear()
+        }


👍 thinking also about better name for viewModel function, maybe startDownloadTasks maybe better? Or any other suggestions?

NetworkingSampleApp/NetworkingSampleApp/Scenes/Download/DownloadProgressViewModel.swift

cejanen · 2023-05-24T17:09:47Z

Sources/Networking/Core/DownloadAPIManager.swift

+    public func urlSession(_: URLSession, downloadTask: URLSessionDownloadTask, didWriteData _: Int64, totalBytesWritten: Int64, totalBytesExpectedToWrite: Int64) {
+        downloadStateDict[downloadTask]?.downloadedBytes = totalBytesWritten
+        downloadStateDict[downloadTask]?.totalBytes = totalBytesExpectedToWrite
+    }


👍 similar we have for running calls and retry mechanism

Sources/Networking/Misc/URLSessionTask+asyncResponse.swift

tonyskansf

The crash is now fixed with the actors 👍

From further testing, the memory graph showed that there is a memory leak — the DownloadAPIManager is not being released.

This is what I've changed to test it:

Initialize the DownloadAPIManager only once in the DownloadsViewModel and pass this instance down to the DownloadProgressViewModel instead of using shared.

Going from the examples list to Downloads and back (without tapping "Download") will cause the memory leak. This seems to be the source:

urlSession = URLSession(
    configuration: urlSessionConfiguration,
    delegate: self,
    delegateQueue: OperationQueue()
)

…anager

gajddo00 · 2023-05-25T13:55:39Z

By design, the DownloadAPIManager should be used as a singleton object. URLSession keeps strong reference and Apple also recommends using a shared urlSession, otherwise we'll need the user to free the urlSession manually by calling either

urlSession.finishTasksAndInvalidate()
urlSession.invalidateAndCancel()

I added this as an option in case somebody would like to use a non-singleton instance in the future so they could invalidate the session manually by calling

deinit {
      downloadAPIManager.invalidateSession(true)
}

Give it a try 👍

brenovaladao · 2023-05-30T10:54:33Z

When testing the app on the simulator, when I tap on the download button it always starts the download twice, is it the intended behavior? - I see there are 2 tasks in the VM, so I think it's 🤔 in that case, I'd rather put this in a more explicit way in the method name / button copy

gajddo00 · 2023-05-30T15:33:21Z

Yes, that was just exemplary to test out the support for simultaneous downloads. Let's keep it as the original - one download showcase.

cejanen

Another round of minor updates

NetworkingSampleApp/NetworkingSampleApp/Scenes/Download/DownloadsViewModel.swift

Sources/Networking/Core/DownloadAPIManager.swift

brenovaladao

👍🏼

brenovaladao · 2023-05-31T07:01:53Z

Sources/Networking/Core/DownloadAPIManager.swift

+            do {
+                /// If retry fails (retryCount is 0 or Task.sleep thrown), catch the error and process it with `ErrorProcessing` plugins.
+                try await sleepIfRetry(
+                    for: error,
+                    endpointRequest: endpointRequest,
+                    retryConfiguration: retryConfiguration
+                )
+
+                return try await downloadRequest(
+                    endpointRequest,
+                    resumableData: resumableData,
+                    retryConfiguration: retryConfiguration
+                )
+            } catch {
+                /// error processing
+                throw await errorProcessors.process(error, for: endpointRequest)
+            }


maybe this could go in another method

cejanen

I happy with final result. No code is perfect but we enhanced it a lot in PR. @tonyskansf can you just confirm your concerns about strong reference in URLSession delegate are fixed or explained?
Thank you all!

👏

tonyskansf

By design, the DownloadAPIManager should be used as a singleton object. URLSession keeps strong reference and Apple also recommends using a shared urlSession, otherwise we'll need the user to free the urlSession manually

What specific part of the implementation suggests using the DownloadAPIManager as a singleton prior to 4f25236? I thought it was just for the sake of the example app 😄

Out of curiosity and scope of this PR, is it possible to let the concrete implementation of the DownloadAPIManaging handle the URL session instead of leaving this task up to the dev who is error-prone? For example, the URL session could be invalidated upon the deinitialization of the manager instance. That way, the lifecycle of the URL session would be bound to it. 🤔

// DownloadAPIManager
deinit {
    invalidateSession()
}

Sources/Networking/Core/DownloadAPIManaging.swift

Co-authored-by: Tony Ngo <tonyngo.jr@gmail.com>

gajddo00 · 2023-06-05T14:18:53Z

There was no documentation, only Matej's intent I guess, that's why I added that in as well. The thing is, that deinit method of the manager instance will never get called because of the strong reference, so the user has to do it externally. 😮‍💨 I guess the only way would be to create the session every time a download is requested and then destroy it after the download is finished.

tonyskansf

I guess the only way would be to create the session every time a download is requested and then destroy it after the download is finished.

If that would make sense that could be an improvement. I think for now, whoever will use this lib and want this behavior can provide a different DownloadAPIManaging implementation or subclass the one provided.

gajddo00 · 2023-06-05T14:47:24Z

I think I'll try that but in a separate enhancement PR to test if it's a better solution 👍

feat: add download support v2

8a615b9

matejmolnar linked an issue Mar 7, 2023 that may be closed by this pull request

Simplistic download #29

Closed

Base automatically changed from dev to master March 13, 2023 13:07

gajddo00 changed the title ~~Download support v2~~ Download support May 9, 2023

gajddo00 added 2 commits May 9, 2023 10:19

chore: formatting, ui buttons not displayed if task completed

eb47a4d

chore: extract retry handling to a protocol

83314b1

gajddo00 changed the base branch from master to dev May 9, 2023 08:45

gajddo00 marked this pull request as ready for review May 9, 2023 08:45

gajddo00 added the enhancement New feature or request label May 9, 2023

gajddo00 requested a review from cejanen May 9, 2023 08:46

cejanen requested changes May 11, 2023

View reviewed changes

gajddo00 added 2 commits May 12, 2023 10:26

chore: code cleaup, refactor

880aec1

chore: added documentation

3ac8b59

gajddo00 requested a review from cejanen May 12, 2023 09:13

brenovaladao reviewed May 24, 2023

View reviewed changes

tonyskansf suggested changes May 24, 2023

View reviewed changes

cejanen requested changes May 24, 2023

View reviewed changes

gajddo00 added 2 commits May 25, 2023 10:06

chore: code cleanup, thread safe dictionary

29296ea

chore: code cleanup

a677982

gajddo00 requested review from tonyskansf and cejanen May 25, 2023 08:14

tonyskansf reviewed May 25, 2023

View reviewed changes

chore: add support for manual url session invalidation for download m…

d6599ea

…anager

chore: added recommendation doc to download managing

4f25236

gajddo00 requested a review from brenovaladao May 29, 2023 07:31

chore: keep only one download in the sample viewmodel

fcea64f

cejanen requested changes May 30, 2023

View reviewed changes

code cleanup

3dfc083

gajddo00 requested a review from cejanen May 31, 2023 06:40

brenovaladao previously approved these changes May 31, 2023

View reviewed changes

cejanen previously approved these changes Jun 4, 2023

View reviewed changes

tonyskansf reviewed Jun 5, 2023

View reviewed changes

Sources/Networking/Core/DownloadAPIManaging.swift Outdated Show resolved Hide resolved

Update Sources/Networking/Core/DownloadAPIManaging.swift

f00c16c

Co-authored-by: Tony Ngo <tonyngo.jr@gmail.com>

gajddo00 dismissed stale reviews from cejanen and brenovaladao via f00c16c June 5, 2023 14:15

fix: argument label

5010fe6

tonyskansf approved these changes Jun 5, 2023

View reviewed changes

gajddo00 merged commit a0821d0 into dev Jun 6, 2023

gajddo00 deleted the feat/download-support-v2 branch June 6, 2023 07:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download support #33

Download support #33

matejmolnar commented Mar 7, 2023 •

edited by gajddo00

Loading

cejanen left a comment

brenovaladao left a comment

brenovaladao May 24, 2023

cejanen May 24, 2023

gajddo00 May 25, 2023 •

edited

Loading

brenovaladao May 24, 2023

gajddo00 May 25, 2023 •

edited

Loading

tonyskansf left a comment

tonyskansf May 24, 2023

cejanen May 24, 2023

gajddo00 May 25, 2023

cejanen left a comment

cejanen May 24, 2023

cejanen May 24, 2023

tonyskansf left a comment

gajddo00 commented May 25, 2023 •

edited

Loading

brenovaladao commented May 30, 2023 •

edited

Loading

gajddo00 commented May 30, 2023

cejanen left a comment

brenovaladao left a comment

brenovaladao May 31, 2023

cejanen left a comment

tonyskansf left a comment

gajddo00 commented Jun 5, 2023 •

edited

Loading

tonyskansf left a comment

gajddo00 commented Jun 5, 2023 •

edited

Loading

Download support #33

Download support #33

Conversation

matejmolnar commented Mar 7, 2023 • edited by gajddo00 Loading

cejanen left a comment

Choose a reason for hiding this comment

brenovaladao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gajddo00 May 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gajddo00 May 25, 2023 • edited Loading

Choose a reason for hiding this comment

tonyskansf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cejanen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyskansf left a comment

Choose a reason for hiding this comment

gajddo00 commented May 25, 2023 • edited Loading

brenovaladao commented May 30, 2023 • edited Loading

gajddo00 commented May 30, 2023

cejanen left a comment

Choose a reason for hiding this comment

brenovaladao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cejanen left a comment

Choose a reason for hiding this comment

tonyskansf left a comment

Choose a reason for hiding this comment

gajddo00 commented Jun 5, 2023 • edited Loading

tonyskansf left a comment

Choose a reason for hiding this comment

gajddo00 commented Jun 5, 2023 • edited Loading

matejmolnar commented Mar 7, 2023 •

edited by gajddo00

Loading

gajddo00 May 25, 2023 •

edited

Loading

gajddo00 May 25, 2023 •

edited

Loading

gajddo00 commented May 25, 2023 •

edited

Loading

brenovaladao commented May 30, 2023 •

edited

Loading

gajddo00 commented Jun 5, 2023 •

edited

Loading

gajddo00 commented Jun 5, 2023 •

edited

Loading