Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds option to stop background download controller reading to disk #44

Merged
merged 2 commits into from
Apr 8, 2020

Conversation

simonmitchell
Copy link
Contributor

@simonmitchell simonmitchell commented Apr 7, 2020

Added this so we can (try and) avoid Background download daemon's 40mb memory limit!

Description

Added a property to background controller init method to stop it reading downloaded file to Data immediately.

How Has This Been Tested?

Has been tested with the implementation of background downloading storm delta bundles in ThunderCloud

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

so we can (try and) avoid Background download daemon's 40mb memory limit!
@@ -27,15 +27,21 @@ public class BackgroundRequestController: NSObject, URLSessionDelegate, URLSessi

private var urlSession: URLSession?

private let readData: Bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this is needed as in instance property

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need it to check on line 66 if we're going to read the file into Data :D

Copy link
Contributor

@BenShutt BenShutt Apr 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry Github was hiding code an thought it was part of the init, my bad, that was a bit silly :P

@@ -56,7 +62,11 @@ public class BackgroundRequestController: NSObject, URLSessionDelegate, URLSessi
responseHandler?(downloadTask, nil, nil)
return
}
let response = RequestResponse(response: taskResponse, data: try? Data(contentsOf: location))
var data: Data?
if readData {
Copy link
Contributor

@BenShutt BenShutt Apr 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to save a (possibly) unnecessary read :).

Makes sense why URLSessionDownloadTask only returns a file URL.
Then again surely URLSessionDownloadTask is more or less the same as an URLSessionDataTask just saving the Data to a file on successful response. So a URLSessionDataTask then writing that data manually in the callback is the same time overall?

Anyway here we are using URLSessionDownloadTask so that read doesn't need to be done into Data here as per your change! :) Instead for that the user is better using a URLSessionDataTask.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think URLSessionDataTask would be incrementally saving to disk on download (Certainly with the app in the background) so unsure of that, but just trying to do everything we can from our side to reduce the memory footprint as there is a much discussed (But not documented) 40mb seemingly with urlsessiond which handles background downloads :D

URLSessionDownloadTask is required in this particular case because URLSessionDataTask won't execute in the background :)

@BenShutt
Copy link
Contributor

BenShutt commented Apr 8, 2020

Maybe a nice feature for the future would be an improved response model based on what you're doing.
For example a DataTask Result<Data, Error>
For example a DownloadTask Result<FileResponse, Error>
where

struct FileResponse {
    var fileURL: URL

    // ...

    func data() throws -> Data {
        return try Data(contentsOf: fileURL)
    }

    // ...
}

enum for Error is powerful so we could do a lot round that

@simonmitchell
Copy link
Contributor Author

I'd love to make the network responses more swifty for sure! Using Result is my ideal approach, in a very similar way to what you have above :D I'm just wary as this will cause us millions of breaking changes across all our apps 😂

@simonmitchell simonmitchell merged commit 742e52b into develop Apr 8, 2020
@simonmitchell simonmitchell deleted the fix/background_download_memory_limits branch April 8, 2020 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants