matteom / Podcast

MIT License
17 stars 4 forks source link

There's a gotcha in this code 😹 #1

Open iandundas opened 1 year ago

iandundas commented 1 year ago

Really loved the blog post, however there's actually a gotcha in this approach using continuations.

func urlSession(_ session: URLSession, downloadTask: URLSessionDownloadTask, didFinishDownloadingTo location: URL)

Unfortunately the location is "use it or lose it" .. before the function exits scope. Otherwise the file at this temporary location no longer exists.

CleanShot 2023-04-12 at 13 47 12@2x docs

So if we modify ViewModel.saveFile(for episode: Episode, at url: URL) to actually catch the error:

CleanShot 2023-04-12 at 13 48 39@2x

Then the error is revealed:

error: Error Domain=NSCocoaErrorDomain Code=4 "“CFNetworkDownload_45xjff.tmp” couldn’t be moved to “1386867488” because either the former doesn’t exist, or the folder containing the latter doesn’t exist." UserInfo=...., NSUnderlyingError=0x600001f0a550 {Error Domain=NSPOSIXErrorDomain Code=2 "No such file or directory"}}

Basically the above function calls continuation?.yield(.success(url: location)) and then returns, before the ViewModel has a chance to actually handle the next event (i.e. before it has a chance to move the file out of the temporary location). So the file is gone.

Quite heartbreaking because this was otherwise a very nice approach. 💔

Not sure if there's a way to tweak it so that the continuation is handled synchronously, I'm not that advanced with async/await yet.

matteom commented 1 year ago

Thanks for the report. However, I cannot reproduce the problem. Did you run the project from this repo or did you make yours following the article?

The problem you have might be caused by the thread where the continuation runs. In my test, the urlSession(_:downloadTask:didFinishDownloadingTo:) method of the Download class runs on the main thread. Thus, when it yields the continuation, the for await loop in the view model and, subsequently, the saveFile(for:at:) method of the ViewModel class also run on the main thread.

That means that, at that point, the urlSession(_:downloadTask:didFinishDownloadingTo:) did not return yet, and the file is still present.

If you created your own project, you might have forgotten to mark the download(_:) method of ViewModel with @MainActor. If, instead, you are running the project from this repo, it might be that the delegate callbacks come on random threads when you run the app. That probably depends on the internal implementation of URLSessionDownloadTask.

The solution is making sure that urlSession(_:downloadTask:didFinishDownloadingTo:) runs on the main thread. Marking it with @MainActor should work, but I haven't tried. If that fails, use the main dispatch queue to yield the continuation:

func urlSession(_ session: URLSession, downloadTask: URLSessionDownloadTask, didFinishDownloadingTo location: URL) {
    DispatchQueue.main.sync {
        continuation?.yield(.success(url: location))
        continuation?.finish()
    }
}
iandundas commented 1 year ago

Hey @matteom, sorry for the huge delay to your reply. Just snowed under đŸ‘Ș. Thanks for your suggestions. From memory I experienced it in my own code, and then went to try the sample code for the blog post.

This is a bit outlandish but here's a video of the few steps I just took now

đŸŽ„ Video: http://172.104.253.215/CleanShot-2023-06-02-at-13.01.10-converted.mp4

Basically:

one wrinkle is that the first download didn't log an error, but subsequent downloads all logged the following:

Error Domain=NSCocoaErrorDomain Code=4 "“CFNetworkDownload_xht5ww.tmp” couldn’t be moved to “1386867488” because either the former doesn’t exist, or the folder containing the latter doesn’t exist." UserInfo={NSSourceFilePathErrorKey=/Users/ian/Library/Developer/CoreSimulator/Devices/B8576463-318E-4269-B851-9BD51900BF2F/data/Containers/Data/Application/227A6458-FA08-4A0A-88BD-224D6CCE74E5/tmp/CFNetworkDownload_xht5ww.tmp, NSUserStringVariant=(
    Move
), NSDestinationFilePath=/Users/ian/Library/Developer/CoreSimulator/Devices/B8576463-318E-4269-B851-9BD51900BF2F/data/Containers/Data/Application/227A6458-FA08-4A0A-88BD-224D6CCE74E5/Documents/1386867488/1000614310838.mp3, NSFilePath=/Users/ian/Library/Developer/CoreSimulator/Devices/B8576463-318E-4269-B851-9BD51900BF2F/data/Containers/Data/Application/227A6458-FA08-4A0A-88BD-224D6CCE74E5/tmp/CFNetworkDownload_xht5ww.tmp, NSUnderlyingError=0x6000030db720 {Error Domain=NSPOSIXErrorDomain Code=2 "No such file or directory"}}
iandundas commented 1 year ago

If I make two changes to add print statements:

    func saveFile(for episode: Episode, at url: URL) {
        print("Exists in `ViewModel`?: ", FileManager.default.fileExists(atPath: url.path()))

and

func urlSession(_ session: URLSession, downloadTask: URLSessionDownloadTask, didFinishDownloadingTo location: URL) {
        print("Exists in `Download`? : ", FileManager.default.fileExists(atPath: location.path()))

I (almost) always get:

Exists in `Download`? :  true
Exists in `ViewModel`?:  false

thought occasionally both have been true. Strange..

lazygunner commented 1 month ago

I'm facing the same problem, and the "main thread solutions" not work either. And then I tried to solve it by a tricky way. I copy the tmp file to "Documents" directory in the delegate function, and return the new URL. func urlSession(_ session: URLSession, downloadTask: URLSessionDownloadTask, didFinishDownloadingTo location: URL) { // copy the file from tmp to Documents let newURL = copyFileToDocuments(from: location) continuation?.yield(.success(url: newURL!)) continuation?.finish() }