I'm playing around with Combine and I've found a possible memory leak when using 'assign(to:)' to store stream result in current object. Let me show a couple of examples using 'assign(to:)' and 'sink' to compare:
final class Bar: ObservableObject {
@Published var input: String = ""
@Published var output: String = ""
private var subscription: AnyCancellable?
init() {
subscription = $input
.filter { $0.count > 0 }
.map { "\($0) World!" }
.sink { [weak self] input in
self?.output = input
}
}
deinit {
subscription?.cancel()
print("\(self): \(#function)")
}
}
// Ussage
var bar: Bar? = Bar()
let foo = bar?.$output.sink { print($0) }
bar?.input = "Hello"
bar = nil
foo?.cancel()
When executed this code produces:
Hello World!
__lldb_expr_29.Bar: deinit
But, when I use 'assign', deinit is never called:
final class Bar: ObservableObject {
@Published var input: String = ""
@Published var output: String = ""
private var subscription: AnyCancellable?
init() {
subscription = $input
.filter { $0.count > 0 }
.map { "\($0) World!" }
.assign(to: \.output, on: self)
}
deinit {
subscription?.cancel()
print("\(self): \(#function)")
}
}
// Ussage
var bar: Bar? = Bar()
let foo = bar?.$output.sink { print($0) }
bar?.input = "Hello"
bar = nil
foo?.cancel()
This only prints:
Hello World!
So, my question here is: Am I doing something wrong with 'assign' or there is a real memory leak with it?
Indeed there is a retain cycle with the sample code that uses assign:
The Bar retains the AnyCancellable (1), which retains the Subscribers.Assign subscription (2), which retains the Bar (3) in order to be able to perform the assignment. Cycle completed: the Bar is never deinited.
Your other sample code captures self weakly. This avoids (3), breaks the cycle, and fixes the memory leak.
Subscribers.Assign<Root, Input> indeed requires a ReferenceWritableKeyPath, a "key path that supports reading from and writing to the resulting value with reference semantics". But the Root generic argument is not constrained to AnyObject: the subscriber and its subscriptions can not capture the target weakly.
I don't know if this is a flaw or not. But we can reasonably deduce this behavior from the definition of the type, and adjust our expectations.
Maybe, in your specific case, where I assume that only output really needs to be published, input could be a simple property which updates the published output in its didSet?
There is indeed a memory leak if self is both keeping the reference to the subscription and is the target of the assignation. I've made the following replacement that seems to do the trick.
I'm simply proxying the assignation through a sink where I can weakly refer to self. I'm using a Just publisher to let Combine manage the assignation. It seems to work as expected on my side.
extension Publisher where Self.Failure == Never {
public func assignNoRetain<Root>(to keyPath: ReferenceWritableKeyPath<Root, Self.Output>, on object: Root) -> AnyCancellable where Root: AnyObject {
sink { [weak object] (value) in
guard let object = object else { return }
_ = Just(value).assign(to: keyPath, on: object)
}
}
}
extension Publisher where Self.Failure == Never {
public func assignNoRetain<Root>(to keyPath: ReferenceWritableKeyPath<Root, Self.Output>, on object: Root) -> AnyCancellable where Root: AnyObject {
sink { [weak object] (value) in
object?[keyPath: keyPath] = value
}
}
}
@tgrapperon et al - many thanks for digging in to all this and providing some simple workarounds. The assign operator seemed great until I ran into this, and substituting sink everywhere seemed ugly.
On a side note, if I understood correctly, this behavior implies that this wouldn't cause the leak right?
class SomeAssignee {
var someData: String
}
class SomeObservable: ObservableObject {
var cancellables: Set<AnyCancellable>()
@Published var observedData: String = ""
init(_ target: SomeAssignee) {
$observedData
.assign(\.someData, on: target)
.store(in: &cancellables)
}
}
That looks suspiciously like a property wrapper / binding / observable wrapper.
It seems Apple did fix this issue in iOS 14, by introducing the assing(to:) operator, which takes a Published.Publisher as its input and instead of returning an AnyCancellable, it binds the lifetime of the subscription to the lifetime of the Published property.
From the docs:
If you instead implemented MyModel with assign(to: lastUpdated, on: self) , storing the returned AnyCancellable instance could cause a reference cycle, because the Subscribers.Assign subscriber would hold a strong reference to self . Using assign(to:) solves this problem.
Important: The Subscribers/Assign instance created by this operator maintains a strong reference to object, and sets it to nil when the upstream publisher completes (either normally or with an error).
in this case, for just, it will complete immediately , so the strong reference was set to nil