Have you actually tested it? In my tests (e.g. of the above app or others) the relevant guard let self block is only entered once on the first notification after the instance was deallocated, but never again.
This is an infinite for loop observing notifications being posted. If you don't enter the loop anymore when new notifications are posted, that means the task was cancelled (which cancelled the async sequence as well). What causes the task to cancel?
This?
1 Like
This is the documentation for func addObserver(Any, selector: Selector, name: NSNotification.Name?, object: Any?), I'm not sure how this applies to func notifications(named: Notification.Name, object: AnyObject?) -> NotificationCenter.Notifications since the latter does not take an observer object as input. I assume the former captures the observer that is passed in as weak and cancels the observation the next time it notices the weak reference is nil. But with the async sequence that we have here, there is no such observer object and the async sequence needs to be cancelled.
But actually I just realized what I was missing: the guard breaks the infinite loop because it returns from it when the weak reference is nil. So indeed the observation is cleaned up the next time a notification fires. This is not the case with the Combine example that was given above.
1 Like
Can you please show them again as there been quite a few.
This example works fine:
private var cancellable: Cancellable?
...
Task { [weak self] in
guard let self else {
print("no self -> return (1)")
return
}
cancellable = NotificationCenter.default
.publisher(for: UIDevice.orientationDidChangeNotification)
.sink() { [weak self] _ in
guard let self else {
print("no self -> return (2)")
return
}
print("Orientation changed \(self)")
}
}
Interestingly, it works even better than the above notification example: first deinit happens that cleans cancellable cancelling them, so "no self -> return" lines are not even hit.
Indeed this has the nice property of cancelling the subscription immediately and there is no need to wait for another notification to fire (which may not happen for a long time and possibly never). Similarly you should be able to cancel a Swift Concurrency task form the deinit of your object.
It can be misleading to try to use a task this way, though, because even though the initial capture may be weak, if you aren't careful, then almost any actual use of the reference in the task's execution is going to form a strong reference again, such as invoking methods on the object or passing the reference to another function. It is possible to use weak references this way, but it requires care to make sure those strong references at least occasionally go away at points where you want to observe whether the object still has uses outside of the task. As far as the tendency to mechanically write [weak self] on every closure out of fear of reference cycles, though, that really isn't necessary with tasks (and, to be fair, there are often better alternatives for traditional closure-based APIs as well, such as discarding the closure when it's no longer needed, using capture lists to capture the fields of self you need rather than the entire object, etc.)
1 Like
I would be careful with that. It's very easy to introduce bugs this way unless you pay attention to details and you understand what you are doing. Example:
import Foundation
import PlaygroundSupport
PlaygroundPage.current.needsIndefiniteExecution = true
class SomeWrapper {
let value = "1"
}
class SampleObject {
var field: SomeWrapper?
var closure: (() -> ())?
init() {}
func runTest1() {
print("--- Test 1")
closure = { [field] in
print(field?.value ?? "doesn't exist")
}
field = SomeWrapper()
closure?()
}
func runTest2() {
print("--- Test 2")
field = SomeWrapper()
closure = { [field] in
print(field?.value ?? "doesn't exist")
}
field = nil
closure?()
}
func runTest3() {
print("--- Test 3")
closure = { [weak self] in
print(self?.field?.value ?? "doesn't exist")
}
field = SomeWrapper()
closure?()
}
func runTest4() {
print("--- Test 4")
field = SomeWrapper()
closure = { [weak self] in
print(self?.field?.value ?? "doesn't exist")
}
field = nil
closure?()
}
}
let obj = SampleObject()
obj.runTest1()
obj.runTest2()
print("\nExpected behavior:\n")
obj.runTest3()
obj.runTest4()
Output:
--- Test 1
doesn't exist
--- Test 2
1
Expected behavior:
--- Test 3
1
--- Test 4
doesn't exist
In tests 1 and 2 you get something that most likely wouldn't be expected by you. And often the code is more complex than the one posted here, so this issue could be well hidden.
That's why usually it's better to use [weak self] even if it's not really necessary than tracking memory leaks across the whole application with the memory graph. I would think about [weak self] more as a way to say that I don't want this closure to prevent the app from releasing self.
But, of course, there are safe places where you can easily capture a strong reference to self.
1 Like
Although in the example brought up above weak is desired, no?
Task {
// [weak self] in
// guard let self else { return }
let asyncSequence = NotificationCenter.default.notifications(named: UIDevice.orientationDidChangeNotification)
for await _ in asyncSequence {
print("GOT \(self)")
}
}
If I comment weak out my object is not getting deallocated (and the notification observer goes forever).
Yeah, if you want to see modifications to a particular field, then you still need to capture the object containing the field rather than the value of the field itself. You can still do that by putting field in a separate object that doesn't also own the closure. If you do want the value of field to go away and become nil when the object does, then a weak reference is fine; if your only goal with the weak reference is to avoid memory leaks, then the weak reference is adding state space to your program that it doesn't need.
If you uncomment it, though, then you still don't get the behavior you probably want. Either self exists at the point of entry into the task, and the notification observer goes on forever, or it doesn't, and the notification observer never runs at all. You could do this instead:
Task {[weak self] in
let asyncSequence = NotificationCenter.default.notifications(named: UIDevice.orientationDidChangeNotification)
for await _ in asyncSequence {
guard let self else { return }
print("GOT \(self)")
}
}
to check the weak reference on every iteration. In a real program, though, you have to be careful that the actual work which the print statement is standing in for doesn't cause the task to retain strong references to the object. Furthermore, as soon as you have more than one of these task loops running on the same object, it is very likely that at least one of the tasks will be actively using a strong reference to self while another task is checking the weak reference, causing the group of tasks to either never end or end later than you think they should, because the tasks all have to luck into releasing their strong references and checking the weak reference at roughly the same time in order for any of them to see the object die. That's why I say that weak references are rarely helpful in tasks, because the end of an object lifetime is not really a reliable signal for ending or cancelling async work. Weak references can be used carefully, but an explicit ending event is usually more robust. This is however IMO an important problem for us to provide a solution for, but I don't think weak references in their current form are really up to the task.
10 Likes
Yes, we need a better solution to this problem.
1 Like
I am trying to reproduce your example, but maybe I'm doing something wrong?
for _ in 0 ..< 1000 {
Task { [weak self] in
let asyncSequence = NotificationCenter.default.notifications(named: UIDevice.orientationDidChangeNotification)
for await _ in asyncSequence {
guard let self else {
print("breaking cycle")
return
}
try await Task.sleep(nanoseconds: UInt64.random(in: 0 ... 1000_000_000)) // 0 ... 1 second
print("ORIENTATION CHANGED \(self)")
}
}
}
Here object's denit happens normally when it should, after that when notification is triggered I can see a bunch of "breaking cycle" in the console terminating the corresponding tasks.
It's hard to speak with any certainty about the behavior of concurrent code. Maybe you're getting by on the fact that Task {} inherits the enclosing actor context, so all of the tasks are running mutually exclusive with one another. Since the only suspend points in that code the sleep and the for await, each task has maybe a 50/50 chance of being suspended while holding a strong reference during the sleep phase. That likelihood will go down much further if the tasks aren't sharing actor isolation and can run in parallel, or if more actual work with more suspend points is being done in the tasks instead of sleeping.
1 Like