In Chapter 3, in the process of adopting the ByteAccumulator to collect data from URLSession bytewise downloader, the book directs the reader to write this to send out periodic updates:
The [weak self] here immediately surprised me because I don’t think there’s any possibility of a retain cycle. We don’t save off the task to an ivar or anything, which is where it would become a risk of retain cycle if not carefully handled (in that case, self would retain the task, and the task would retain self. Would either need [weak self] or for task to explicitly nil out self’s ivar as part of its execution).
In fact, I would wantself to be retained and kept alive until the final self.updateDownload() has been completed.
The explanation given is also, er, I think handwavy to the point of being misleading for less experienced devs:
That means the usual memory management rules apply when you use escaping closures
When a closure is @escaping, it doesn’t mean at all that it must automatically gain a [weak self], it merely means that the memory management rules must be considered deliberately.
Curious to know if I’m misunderstanding something around Tasks and memory management (are tasks implicitly retained by parent tasks or something? I expect that .detached() would break this chain if so…)
While I’m looking at this code, I have a couple more questions haha:
updateDownload() is a @MainActor method. What are the ramifications of having a .medium priority detached task that immediately re-enqueues onto the main actor?
accumulator.progress is called within the body here, but accumulator could theoretically be partway into processing the next series of bytes, yeah? I imagine it would be safer to calculate and capture the progress outside the body of the Task, like so:
I had similar concerns about this code. Especially about progress, since ByteAccumulator is a class I would expect that it can send wrong result. Is there something magical about detached task?
Maybe let’s see the author @icanzilb would be so kind to explain us this.
All of this is fantastic feedback — thank you so much for taking the time to write everything down!
regarding [weak self], I also don’t immediately see how could the code in the specific task retain the model. I remember that we reworked this part of the ByteAccumulator a whole bunch of times in summer while working on the book, it could be that this is a leftover of a previous version… @freak4pc could you be so nice to have a quick look at this and DM me — I agree we should remove the weak self here.
Regarding the .medium priority, priorities were still in flux while we worked on this edition of the book. I remember that if we didn’t set the priority here explicitly the progress bar ended up never updating (that didn’t really make sense to me either). I’ve played with this today and it seems that this is not the case anymore though — I’ll just add an explanation that the download runs with userInitiated priority.
About accessing accumulator.progress from the task body — this is something we overlooked! Thank you for spotting this, I ran quickly the project with TSan and indeed we have a race here:
WARNING: ThreadSanitizer: Swift access race (pid=5382)
Modifying access of Swift variable at 0x7b1400087970 by thread T1:
#0 ByteAccumulator.append(_:) ByteAccumulator.swift:57 (SuperStorage:x86_64+0x100030d99)
#1 (5) await resume partial function for SuperStorageModel.downloadWithProgress(fileName:name:size:offset:) SuperStorageModel.swift:98 (SuperStorage:x86_64+0x10003f19d)
#2 swift::runJobInEstablishedExecutorContext(swift::Job*) <null>:2 (libswift_Concurrency.dylib:x86_64+0x2b37c)
We will do the changes and merge them upstream asap
