Rxswift: Disposables retain cycle and leak memory

[jdmoreira]

Short description of the issue:

Disposables hold references to themselves creating retain cycles.

Expected outcome:
Disposables don't create retain cycles and get destroyed when out of scope and no other references are holding them.
Other Reactive implementations in the Swift ecosystem don't do this (including Combine).

What actually happens:
Disposables create references to themselves actively leaking memory when not added to a DisposeBag.

Self contained code example that reproduces the issue:

    func applicationDidFinishLaunching(_ aNotification: Notification) {
        func runTest() -> Void {
            let someBytes = Array(repeating: 0, count: 1000)
            _ = Observable<Int>
                .interval(RxTimeInterval.seconds(1), scheduler: MainScheduler.instance)
                .do(onNext: { _ in
                    let captured = someBytes
                    _ = captured
                })
                .subscribe()

            return
        }

        var counter = 1000000
        while counter > 0 {
            counter -= 1
            runTest()
        }
    }

Memory will easily hit multiple Gigabytes.

RxSwift/RxCocoa/RxBlocking/RxTest version/commit

_v5.1.1_

Platform/Environment

• [x] iOS
• [x] macOS
• [x] tvOS
• [x] watchOS
• [x] 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

[jdmoreira]

This even seems to be common knowledge by some members of the community as evidenced by this stack overflow answer.

[freak4pc]

@danielt1263's explanation in SO is accurate as always. As I mentioned in a separate post, as opposed to Combine where disposal happens when the Cancelable itself is deallocated, Disposables are explicitly disposed by either calling dispose on them, or whenever the stream has a terminating event that would cause it to complete (completed or error).

It's a design choice made early in the life of this project, and isn't something we would change with so many apps depending on the existing behavior. Regardless, this behavior is 99% of the time entirely valid, and only require a bit of manual management in very rare cases.

If you have further questions, let me know and I'll reopen this.

[jdmoreira]

Wouldn't it be trivially fixed by having all Disposables call dispose() on deinit?
This has been the case I've been trying to make since the begging of our conversation.

Anyway, I understand it's too late for RxSwift to do the sane thing.

Thanks for you patience.

[freak4pc]

As I mentioned, it's a design choice for this library and we preferred not going down that path. You can easily make your own custom disposable that conforms to Disposable and _does_ handle the deinit case.

[danielt1263]

The change would not be trivial to the hundreds of apps that use RxSwift and depend on the disposable _not_ killing the subscription on deinit. All over my code, I ignore the disposable because I want the subscription to last until the source emits a stop event. If this change was made, my apps would start silently breaking when the library was updated and I would have to create a global dispose bag for the sole purpose of keeping the disposables from disposing.

[jdmoreira]

I understand that. It's a bit hard for me to accept that people would actively ignore disposables and put on themselves the mental burden of guaranteeing all their observables complete.
On the other hand things being destroyed when out of scope is Programming 101 and everyone is conditioned to it.

I know another case where a retain cycle is used - the Future implementation in Flow - but contrary to Observables that have zero guarantees of ever completing and destroying themselves, a Future in Flow always terminates, either with a value or a timeout.
IMHO is totally reasonable to do it in that case.

But I appreciate your efforts and for sure I respect your opinion, I just happen to strongly disagree with this design choice.

Like I said, I also understand it's too late to change this. I came here mostly wanting to have an understanding of the reasons behind it and satisfy my curiosity. It's easily fixable from my side in multiple ways.

Thank you both for your time and explanations.

[freak4pc]

The way ARC works and how Disposable(s) are designed is the main reason we recommend using a DisposeBag when you need to tie deallocation of an owner. Anyways, glad you found at least answers, even if not agreement :)

[danielt1263]

It's a bit hard for me to accept that people would actively ignore disposables and put on themselves the mental burden of guaranteeing all their observables complete.

I'm not sure what observables you are talking about, considering all the ones that come out of RxCocoa and RxSwift _do_ complete (interestingly, the ones associated with UIKit objects all complete when the object goes out of scope.) If you are in the habit of not emitting a completed event after an Observable is complete, then I guess you might have something to worry about... Honestly, it's one of the reasons I avoid using Relays because they can't complete, which I consider to be rather unsafe.

[jdmoreira]

Plenty of Observables don't complete, timers being just one example. and that's my point really, why even have to think hard if some Observable completes or not. Just make sure the Disposable get destroyed (being out of scope would be a start).

UIKit objects all complete when the object goes out of scope

And who guarantees you are not capturing the viewController or some other view in some subscription closure (or a map, flatMap, etc...) thereby creating a memory leak? An all too common scenario I assume.

[danielt1263]

why even have to think hard if some Observable completes or not.

Why have to search all over the code to check if the subscription ends at the right time? To my mind, it's silly to have an imperative cancelation system in an otherwise declarative setup. I grant that some allowances have to be made because UIKit is inherently imperative, but why lean into it?

As an example:

extension UITabBarController {
    func example(menuAction: Observable<MenuAction>) {
        _ = menuAction
            .map { action in
                switch action {
                case .dashboard:
                    return 0
                case .laborVariance:
                    return 1
                case .punchManagement:
                    return 2
                }
            }
            .takeUntil(rx.deallocating)
            .bind(onNext: { [unowned self] index in
                self.selectedIndex = index
            })
    }
}

I like the way disposables work now precisely because I don't have to worry with the above about where to park the disposable.

And who guarantees you are not capturing the viewController or some other view in some subscription closure (or a map, flatMap, etc...) thereby creating a memory leak? An all too common scenario I assume.

Nothing makes that guarantee and if you do that, then the disposable isn't going to help you even if it did dispose on deinit.

[jdmoreira]

Why have to search all over the code to check if the subscription ends at the right time?

I'm far from doing that. In fact that's exactly what I'm trying to avoid by forgetting self-referencing disposables here and there.

To my mind, it's silly to have an imperative cancelation system in an otherwise declarative setup.

If by imperative you mean explicitly calling dispose() then I agree. If you mean having proper scope management then I disagree.

As an example: ...

Why pass the Observable as an argument? Why not...

extension Reactive where Base: UITabBarController {
   var menuAction: Binder<MenuAction > { ... } 
}

and take care of the Disposable at the bind() call site?

Nothing makes that guarantee and if you do that, then the disposable isn't going to help you even if it did dispose on deinit.

You are assuming my viewController would hold the bag or the disposables but that's not true for me.
You can look into Presentation for an idea of where I hold my bags (they are injected in the Result of the materialise and when that one completes everything gets disposed. Practically retain cycle free).