Different and unexpected behaviour between DisposeBag and CompositeDisposable

[jdmoreira]

Short description of the issue: Different and unexpected behaviour between DisposeBag and CompositeDisposable. Potential retain cycle in CompositeDisposable?

Expected outcome:

I would expect them to behave the same.

What actually happens:

CompositeDisposable doesn't seem to dispose it's children.

Self contained code example that reproduces the issue:

This one

        let foo = Disposables.create {
            print("disposed ☠️")
        }
        foo.disposed(by: DisposeBag())

prints "disposed ☠️".

This one

        let foo = Disposables.create {
            print("disposed ☠️")
        }
        CompositeDisposable(disposables: [foo])

doesn't.

RxSwift/RxCocoa/RxBlocking/RxTest version/commit

5.1.1

Platform/Environment

• [x] iOS
• [ ] macOS
• [ ] tvOS
• [ ] watchOS
• [ ] playgrounds

How easy is to reproduce? (chances of successful reproduce after running the self contained code)

• [x] easy, 100% repro
• [ ] sometimes, 10%-100%
• [ ] hard, 2% - 10%
• [ ] extremely hard, %0 - 2%

Xcode version: 12.0

Installation method:

• [x] CocoaPods
• [ ] Carthage
• [ ] Git submodules

I have multiple versions of Xcode installed: (so we can know if this is a potential cause of your issue)

• [ ] yes (which ones)
• [x] no

Level of RxSwift knowledge: (this is so we can understand your level of knowledge and formulate the response in an appropriate manner)

• [ ] just starting
• [ ] I have a small code base
• [x] I have a significant code base

[jdmoreira]

I did some further investigation and it seems like CompositeDisposable doesn't call dispose() on itself on deinit and DisposeBag does.

I would expect them to behave the same. I abstract them with a protocol and would like to use them interchangeably without this causing me trouble.

[freak4pc]

Hey. This is the expected behavior. Disposables don't dispose on deinit unless they're in a bag that calls dispose on them directly. CompositeDisposable is not an exception to this case.

[jdmoreira]

Alright, I wasn't expecting that but maybe you can further clarify something for me which I find even weirder.

func runTest() -> Void {
    let foo = Observable<Int>
        .interval(RxTimeInterval.seconds(1), scheduler: MainScheduler.instance)
        .do(onNext: { print($0) })
        .subscribe()

    CompositeDisposable(disposables: [foo])

    return
}

runTest()

In this case I keep getting the prints from the timer and I can't for the life of me understand who is holding the subscription. CompositeDisposable (the same happens with just the Disposable foo) is out of scope. No one is holding to it. How is the subscription alive? Is it intentionally keeping a reference to itself that only breaks when diposed() is called?

I would expect subscription to get destroyed when no one is holding to the Disposables.

[freak4pc]

Again, disposables don't dispose of themselves on deinit - you need to either hold them in a bag or explicitly store them on the side and dispose them

I'm not sure why the specific code you shared keeps running but to be honest we don't often provide code-level support in GitHub (maybe slack would be better). Could be related to something silly like a debug scheme or the way Composite hold onto the second Disposable.

Regardless, I imagine you know this but doing what you do in this code isn't considered a best practice - try to be more explicit about disposal (with a bag or dispose()) which will also remove many of these doubts.

[jdmoreira]

Thanks for your patience. I understand I should be using a DisposeBag in fact everything works exactly as I expect when I replace CompositeDisposable by a DisposeBag. I'm more interested in what is happening when I don't use a DisposeBag which is completely possible using the exposed API. In fact is even encouraged in the source of DisposeBag since there is a comment above the private dispose() referencing CompositeDisposable as an alternative.

That code is just an example for me to try and understand several situations I'm experiencing in my real codebase. Up until today I always believed you needed to hold to a Disposable in order for it's subscriptions to kept alive... but I did some serious debugging today and that doesn't seem to be the case when I use CompositeDisposable instead of DisposeBag. I would really expect them to be the same since both of them can hold to other Disposables.

[freak4pc]

which is completely possible using the exposed API.

Everything is possible, it doesn't make it correct to do it this way :)

even encouraged in the source of DisposeBag since there is a comment above the private dispose() referencing CompositeDisposable as an alternative.

I think you've misread that comment. The point of it is saying, you should not do disposeBag.dispose() - if for some reason you need to manually call .dispose(), it means you need a Disposable. If you need a Disposable that holds many Disposables — that's a CompositeDisposable.

I would really expect them to be the same since both of them can hold to other Disposables.

I don't know why you would expect them to be the same. If two classes do the same thing, there is no reason to have two classes :)

A DisposeBag is meant to be tied to the lifecycle of some owner object that holds the bag and causes its deinit to clear all of its disposables

A CompositeDisposable is comparable to a regular Disposable - only that it represents many subscriptions and not a single one. It doesn't ensure dispose-on-deinit exactly like Disposable doesn't.

Two different tools for two different purposes.

[jdmoreira]

I understand. We are essentially discussing a design choice there is no issue here.

I would still like to leave a suggestion for a protocol named Disposer. CompositeDisposable could be both a Disposable and a Disposer (holds disposables internally). DisposeBag would only be a Disposer. Just a consideration if you ever consider iterating the API.

I'm still seeing subscriptions being kept alive even though the corresponding disposables are out of scope and have no other references from my side. I believe there are retain cycles inside RxSwift since that’s the only explanation I can find. I will open another issue for that so we can have a separate conversation about it.

[jdmoreira]

Everything is possible, it doesn't make it correct to do it this way :)

Oh and let’s agree to disagree on api design choices 😊

[freak4pc]

The person who defines the right way to use the API is usually the API designer :) Using a CompositeDisposable when you want a DisposeBag isn't the right thing to do. You can disagree with the API design, that's absolutely fine :)

I don't think a protocol makes sense because DisposeBag and CompositeDisposable contextually have no shared meaning. That's why there's the Disposable protocol, for those concrete types that do.

[freak4pc]

PS if you find any reproducible case of a retain cycle (with instrumentation), I'd love to look into it

[jdmoreira]

The person who defines the right way to use the API is usually the API designer :) Using a CompositeDisposable when you want a DisposeBag isn't the right thing to do. You can disagree with the API design, that's absolutely fine :)

In my opinion is called "design" because it's about ergonomics. I don't think the ergonomics are particularly good in this case. But let's drop this and agree to disagree.

The retain cycle issue: https://github.com/ReactiveX/RxSwift/issues/2249