ReactiveX / RxSwift

Reactive Programming in Swift
MIT License
24.37k stars 4.17k forks source link

Observable will not be disposed though it has been deinit #251

Closed frogcjn closed 8 years ago

frogcjn commented 8 years ago
        for _ in 0..<100 {
            let variable = Variable(0)
            variable.subscribe(
                onNext: { value in
                    print(value)
                },
                onDisposed: {
                    print("dispose")
                }
            )
           //without disposeBag here
           //each variable will be deinit here
        }
    // there is no "dispose" appeared on console

Without disposeBag, and when the observable already deinit, the subscription will not be disposed. I think this is an unexpected behavior.

When the observable deinit, the subscription should be disposed, though it is not completed and not specifically add to a disposeBag.

yury commented 8 years ago

@frogcjn yes, it is a little bit confusing... Right now, you need to use ScopedDisposable

   variable.subscribe(
        onNext: { value in
            print(value)
        },
        onDisposed: {
            print("dispose")
        }
   ).scopedDispose()
kzaher commented 8 years ago

Hi @frogcjn ,

concept itself doesn't know anything about ARC, but it's not really that much useful if it did.

This isn't actually so weird as it might seem at start. For example, even if nobody is holding a strong reference to NSTimer, NSOperationQueue, dispatch_queue_t, it will continue to produce elements unless manually cancelled.

We did try to add that Variable on deinit sent Completed, but it just ended up confusing people, and it wasn't really that useful to start with.

It would only help people in those cases that were unidiomatic to start with (using a lof of manual subscribe methods and chaining results manually instead of chaining using operators), so I've decided not to go that road.

All ControlProperty and ControlEvent do send Completed when deallocated in RxCocoa. But we are still recommending usage of dispose bags to terminate subscriptions because observables sequences abstract any kind computation and you can't be certain will it terminate after some time or not.

And yes, it can work in some cases without disposeBag, but you are not making your code future proof :)

frogcjn commented 8 years ago

@yury, This is just a simplified code. The variable will be used outside the scope, and the subscription will be used outside the scope too. So the scopedDispose() won't help. I want the subscription disposed only when the variable deinit somewhere, not limited at the end of the block.

for example, if I use scopedDispose():

    var variables = [Variable<Int>]()
    public override func viewDidAppear(animated: Bool) {
        for _ in 0..<100 {
            let variable = Variable(0)
            variables.append(variable)
            variable.subscribe(
                onNext: { value in
                    print(value)
                },
                onDisposed: {
                    print("dispose")
                }
                ).scopedDispose()
        }
    }

at this time, I don't want to the subscription dispose, because the variable live. I just want the subscription dispose at the time the variable dead.

frogcjn commented 8 years ago

@kzaher I still believe the variable or other observable should dispose its subscriptions after they are dealloc. Because the subscription is meaningful only if the signal source is alive.

In Rx.playground, this example shows how to use variable:

    example("Variable") {
        let variable = Variable("z")
        writeSequenceToConsole("1", sequence: variable)
        variable.value = "a"
        variable.value = "b"
        writeSequenceToConsole("2", sequence: variable)
        variable.value = "c"
        variable.value = "d"
    }
    func writeSequenceToConsole<O: ObservableType>(name: String, sequence: O) {
        sequence
        .subscribe { e in
            print("Subscription: \(name), event: \(e)")
        }
    /* or, you can check whether the resource is disposed using this code:
        sequence
        .subscribe(onNext: {e in
            print("Subscription: \(name), event: \(e)")
            }, onDisposed: {
                print("dispose")
        })
    */
    }

This code give me a "hint" that the variable's deinit will dispose the subscription. But in reality, after running this code, there will be subscription resources undisposed -- "zombies"!! It will live in the memory forever!! Though the variable itself is released.

I know it is good for every subscription to add dispose bag, manually cancelled at the end of the subscription, but I think it is also important that the signal source can dispose the subscriptions at the time it dead.

kzaher commented 8 years ago

Hi @frogcjn ,

I think there are multiple issues here, so let's go through them one by one.

That example code

    example("Variable") {
        let variable = Variable("z")
        writeSequenceToConsole("1", sequence: variable)
        variable.value = "a"
   ...

obviously has bug, thnx for reporting that, we need to fix that immediately so we don't confuse new users :+1:

I understand why you think Variable should Complete on dealloc, but let me try to explain why that's not a good idea, and how you can create your own version of Variable that has those features.

Typical implementation of operator looks like approx like this:

extension ObservableType {
    public func myFancyOperator()
        -> Observable<E> {
        return create { observer in 
             return self.subscribe { event in
                     .....
             }
         }
    }
}

That means that downstream operators and observers could retain source observable while there is some subscriber.

If the producer of sequence elements and Observable interface are single object (in the above example self = Variable), that object could potentially never deallocate if somebody subscribes.

Yes, we could make our internal operators not do that, but we can't make those guarantees for all operators out there.

HOWEVER, you can ofc use Rx to implement your desired behavior. It's not so complicated:

// MagicVariable that sends Completed on deinit and cleans resources
public class MagicVariable<Element> {

   // This is the method where all the magic is 
   // Note, producer (MagicVariable) and observable are **two distinct objects**
    public static func createProducerObservablePair(initialValue: E) -> (MagicVariable<E>, Observable<E>) {
       let magicVariable = MagicVariable(initialValue)
       return (magicVariable, magicVariable._subject)
    }

    deinit {
      _subject.on(.Completed)
    }

    private let _subject: BehaviorSubject<Element>

    private var _lock = SpinLock()

    // state
    private var _value: Element

    public var value: Element {
        get {
            _lock.lock(); defer { _lock.unlock() }
            return _value
        }
        set(newValue) {
            _lock.lock()
            _value = newValue
            _lock.unlock()

            _subject.on(.Next(newValue))
        }
    }

    init(_ value: Element) {
        _value = value
        _subject = BehaviorSubject(value: value)
    }
}

HOWEVER, I wouldn't suggest thinking and coding that way because I'm assuming people need this feature do think and code this way ...

class MyViewModel {
    var a = Variable(1)
    var b = Variable(1)
    var c = Variable(1)
    init() {
        combineLatest(a, b, resultSelector: +).subscribeNext { cValue in
            c.value = cValue
        }
        // .addToDisposeBag(self.disposeBag) <-- even without this this is ugly code and wrong thing to do :)
    }
}

instead of simply writing

class MyViewModel {
    var a = Variable(1)
    var b = Variable(1)
    var c = combineLatest(a, b, resultSelector: +) // <-- this is proper way to chain disposables, not dispose bags
    init() {
    }
}

Hope this clears things up :)

kzaher commented 8 years ago

I think we can reopen this if there will be some new questions. Closing this for now.

frogcjn commented 8 years ago

OK.

arctouch-marcelogobetti commented 6 years ago

Hi @kzaher , while reading #1501 I got curious about Variable having "an inconsistent memory management model compared to other parts of RxSwift" and Google sent me to the discussion here. I see that initially it wasn't considered "a good idea" having Variables to complete on dealloc. Because our project is still running RxSwift 3.x and we're having memory issues, I'm interested on understanding the reasons that made it a good idea so I could possibly decide to either adapt our project or finally update to RxSwift 4. If this is already documented somewhere, could you please just point me to it? Thanks a lot!