Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repeatWithBehavior has memory leak #250

Open
tospery opened this issue Aug 17, 2020 · 15 comments
Open

repeatWithBehavior has memory leak #250

tospery opened this issue Aug 17, 2020 · 15 comments

Comments

@tospery
Copy link

tospery commented Aug 17, 2020

repeatWithBehavior has memory leak

@freak4pc
Copy link
Member

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
Copy link
Author

tospery commented Aug 18, 2020

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

@tospery
Copy link
Author

tospery commented Aug 19, 2020

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
Copy link
Author

tospery commented Sep 23, 2020

Does anyone have the same problem?

@vzsg
Copy link
Contributor

vzsg commented Sep 24, 2020

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
Copy link
Contributor

vzsg commented Sep 24, 2020

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

@tospery
Copy link
Author

tospery commented Sep 30, 2020

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

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

@freak4pc
Copy link
Member

freak4pc commented Sep 30, 2020 via email

@vzsg
Copy link
Contributor

vzsg commented Oct 4, 2020

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
Copy link

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
Copy link

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
Copy link
Member

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
Copy link

winstondu commented Jan 17, 2021

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
Copy link

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

@freak4pc
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants