I have a case where a view controller method is calling another method with a closure. Inside that I'm setting up a subscription to a publisher and in sink I'm calling a separate function on the view controller. I'm capturing self weakly in the outer closure and seeing different results based on whether I reference mySelf inside a sink closure vs any other closre.
Here's the view controller's function
// settings is an observable object
// subscriptions is Set()
I have a full iOS sample that demonstrates this in a runnable state.
I don't understand why the latter leaks and the former doesn't. I thought capturing self weak and holding on to it in the outer closure was the "best practice" in this type of situation.
Is this a Combine issue? Swift issue? My misunderstanding?
Question purely about combine is actually more appropriate to be asked over Apple Developer Forums. However, it looks more like reference cycle question. So I will try to answer this.
So the offending code is this:
mySelf.settings.$isEnabled
.dropFirst()
.sink { _ in // Closure X
mySelf.bar()
}
.store(in: &mySelf.subscriptions)
Do you intend to have the following?
mySelf owns mySelf.subscription,
mySelf.subscriptions owns the subscription chain,
subscription chain owns the closure X, and
Closure X owns myself.
Unless you intend to break the cycle by clearing mySelf.subscriptions at some point later on, it looks very cyclical (and therefore leaky) to me.
I don't know about that. Arbitrarily picking the owner based on its position in the code sounds pretty bad to me.
The chain is intended to be owned by a stored property on the view controller var subscriptions = Set<AnyCancellable>() which will be cleared when this VC is deinit’d.
What I don’t understand is why this subscription closure capturing mySelf causes a leak but the closure inside barAsync does not.
It’s not arbitrary. If I were to capture self weakly in a nested closure that would actually introduce a leak because the outer closure would capture it.
It needs to be cleared beforedeinit. Otherwise, there'd be a cycle there as I laid it out above. I don't know what barAsync is doing as I don't see its code, but AFAICT, it might just be holding onto the closure until it is executed, at which point the closure is relinquish, which would be totally fine since that'd be the point where the cycle is broken.
I don't think it says what you said. The whole point of that post seems to be just to demonstrate that weak self in the inner closure requires the outer closre to capture [defaulted to strong] self. It may even be utilizable in some cases if that's what the logic dictates.
Without seeing the implementation of barAsync that’s a bit hard to say, but I’m assuming based on the name that it basically just passes the closure along to some DispatchQueue.async call. If that’s the case, the cycle is broken by GCD itself—the closure passed to async is released after being run. If the work is submitted to a global queue, there’s never any cycle at all.
Most of that type of thing should be fixed in the 2020 OSes, as they've overhauled how Combine manages subscriptions and cancellations (changing behavior in the process, unfortunately).
Combine will now no longer relay values from upstream after cancellation. As soon as you cancel the subscription, Sink (or whatever produced the AnyCancellable) is also cancelled and no additional values will go through. While this is perhaps a more spec-conforming behavior, it makes it far more difficult to use AnyCancellable to cancel long running tasks if you wanted some final output. For instance, Alamofire's Request types produce a final completion value when the Request is cancelled and we were able to relay it through our publishers in the previous versions. Now, however, the value is just lost, making it difficult for Combine users to know the Request has properly cancelled.
Still, the OP's code seems unrelated to that, though. It's still leaking unless the ownership relation between publisher, subscriber, subscription, and cancellable changes.
The provided code has a leak as was suggested above. More over, it creates a new subscription each time the foo method completion is called which might not be desirable. Instead, capture the cancelable into a separate optional variable, call cancel before assigning new cancelable, use second weak reference in the internal observer.
I wasn't aware of this change being made but to be entirely honest - it seems thee fact it was there from the beginning is strange as it allows for something that feels quite tacky. Canceling something should do just that - cancel, and not provide any additional content.
Quite interesting though! Any chance you have a link to the discussion where this change was introduced?