inamiy / RxProperty

A get-only `BehaviorRelay ` that is (almost) equivalent to ReactiveSwift's `Property`
MIT License
85 stars 8 forks source link

Possible undisposed subscription memory leak #10

Open elfenlaid opened 4 years ago

elfenlaid commented 4 years ago

I wonder whether ignoring the bind`s disposable leads to a leak.

    /// Initializes with `initial` element and then `observable`.
    public init(initial: E, then observable: Observable<E>) {
        _behaviorRelay = BehaviorRelay(value: initial)

        _ = observable
            .bind(to: _behaviorRelay)
            // .disposed(by: disposeBag)    // Comment-Out: Don't dispose when `property` is deallocated
    }

Specifically if Observable is never-ending one?

seanchas116 commented 4 years ago

I think it's because after Property itself is deallocated existing subscriptions of the property will stop working (https://github.com/inamiy/RxProperty/pull/6).

I also experienced memory leaks in RxProperty and I ended up with writing another Property implementation that just wraps Driver:

import Foundation
import RxSwift
import RxCocoa

class Property<Element> {
    private var _value: Element!
    private let _driver: Driver<Element>
    private let disposeBag = DisposeBag()

    var value: Element {
        return _value
    }

    init(_ driver: Driver<Element>) {
        _driver = driver
        driver.drive(onNext: { [unowned self] in
            self._value = $0
        }).disposed(by: disposeBag)
    }

    func asDriver() -> Driver<Element> {
        return _driver
    }
}
elfenlaid commented 4 years ago

Ah, some dangerous shenanigans around explicitly unwrapped value 🙈

We accepted property nature as is and ended up with the next:

/// `Property` is a read-only wrapper for `BehaviorRelay`.
///
/// Unlike `BehaviorRelay` it can't accept values
public final class Property<Element> {
    public var value: Element {
        relay.value
    }

    public convenience init(value: Element) {
        self.init(relay: BehaviorRelay(value: value))
    }

    public init(relay: BehaviorRelay<Element>) {
        self.relay = relay
    }

    public func bind(to handler: @escaping (Element) -> Void) -> Disposable {
        relay.observeOn(MainScheduler.instance).subscribe(onNext: handler)
    }

    public func asObservable() -> Observable<Element> {
        relay.observeOn(MainScheduler.instance)
    }

    private let relay: BehaviorRelay<Element>
}

public extension BehaviorRelay {
    func asProperty() -> Property<Element> {
        Property(relay: self)
    }
}

So viewModel uses BehaviorRelay under the hood and exposes properties as a public interface.

The saddest thing here is the loss of composability. Property can't be mapped, filtered, etc. Which is the bread and butter of the Rx approach.

Though Relay and Subject are deviations from core primitives on their own and it's only an unavoidable consequence. Yet, ReactiveCocoa has solved the same issue with its Property/MutableProperty. The hope is not lost.