RxSwiftCommunity / RxSwiftExt

A collection of Rx operators & tools not found in the core RxSwift distribution
MIT License
1.32k stars 213 forks source link

repeatWithBehavior has memory leak #250

Open tospery opened 4 years ago

tospery commented 4 years ago

repeatWithBehavior has memory leak

freak4pc commented 4 years ago

Hey, this isn't a valid Issue. Please provide many more details, analytics, reproduction, etc. If you can provide a PR to fix it, that would be best.

tospery commented 4 years ago

image i repeat a simple observable sequence, and found the memory will grow

tospery commented 4 years ago

Hey, this isn't a valid Issue. Please provide many more details, analytics, reproduction, etc. If you can provide a PR to fix it, that would be best.

The demo as above, can you help me, thanks

tospery commented 3 years ago

Does anyone have the same problem?

vzsg commented 3 years ago

I copied your example into a project (it was pretty cumbersome considering you uploaded a screenshot, not the code itself), and I can confirm that memory usage steadily increases while the subscription is alive and repeating itself. Especially after changing the interval to 0.001 seconds instead of 1.

image

From the Allocations instrument, it seems that the concat used to implement repeatWithBehavior is spawning new Observables and keeping them in memory.


Here is the whole class so no one else has to type it in:


import UIKit
import RxSwift
import RxSwiftExt

class ViewController: UIViewController {
    let disposeBag = DisposeBag()

    override func viewDidLoad() {
        super.viewDidLoad()
        // Do any additional setup after loading the view.
    }

    override func viewDidAppear(_ animated: Bool) {
        super.viewDidAppear(animated)
        repeatTest()
    }

    private func repeatTest() {
        Observable.just(true)
            .repeatWithBehavior(.delayed(maxCount: UInt.max, time: 0.001))
            .subscribe(onNext: { _ in
                print("ping")
            })
            .disposed(by: disposeBag)
    }
}
vzsg commented 3 years ago

Here is the full project for quicker repro: RepeatTest.zip.

tospery commented 3 years ago

Here is the full project for quicker repro: RepeatTest.zip.

Thanks your reply, do you have any idea to solve this problem?

freak4pc commented 3 years ago

I tried to debug it but couldn’t immediately find a solution. I’ll see if I can find some more time but this is a bit of a packed period of time.

Shai On Sep 30, 2020, 6:03 AM +0300, 杨建祥 notifications@github.com, wrote:

Here is the full project for quicker repro: RepeatTest.zip. Thanks your reply, do you have any idea to solve this problem? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

vzsg commented 3 years ago

Here is the full project for quicker repro: RepeatTest.zip.

Thanks your reply, do you have any idea to solve this problem?

None, sadly. All I could do is pop in and add the missing details of the issue.

Perhaps as a temporary measure, you could replace repeat with an interval + flatMapFirst.

winstondu commented 3 years ago

We ran into this problem as well, leading to a stackOverflow.

        // This will crash before 1000 is printed.
        let v = DisposeBag()
        Observable.just(10)
            .flatMapLatest { num -> Observable<Int> in
                let repeatingEvents = Observable<Int>.just(num).repeatWithBehavior(
                    .customTimerDelayed(maxCount: .max, delayCalculator: { return .milliseconds(100 + $0)
                    })
                ).enumerated()

                return repeatingEvents.map { return $0.0 + 1
                }
            }
            .bind {
                if $0 % 10 == 0 {
                    print($0)
                }
            }.disposed(by: v)
winstondu commented 3 years ago

We fixed this problem by rewriting repeatWithBehavior.

This is our solution for RxSwift 5.1.1. While I'm not completely sure it doesn't leak, we were no longer seeing a clear increase in memory over time.

Feel free to adapt back to official solution @freak4pc

https://gist.github.com/winstondu/91da8a889ed4e42ef7b1c11951cd3edd

freak4pc commented 3 years ago

Thank you! Any chance you can make a PR out of this, so it's a bit easier to review the changes you made in your rewrite? Appreciate your help!

winstondu commented 3 years ago

Thank you! Any chance you can make a PR out of this, so it's a bit easier to review the changes you made in your rewrite? Appreciate your help!

Unfortunately this solution is based on RxSwift 5.1.1, not the current head of RxSwift (6). We may not migrate our codebase to RxSwift 6 for a while.

I can still make the PR, but only if you also make a branch and new release for RxSwiftExt 5.2

winstondu commented 3 years ago

@freak4pc , I can't even build the repo right now. Carthage does not work for XCode 12: https://github.com/Carthage/Carthage/pull/3106

freak4pc commented 3 years ago

Yup, unfortunately aware of that :( Need to use the SPM/CocoaPods alternatives in this case. Might have to update the demo project.