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

Retain cycle in bind(to:input:) #227

Open
nflahavan opened this issue Jan 24, 2021 · 5 comments
Open

Retain cycle in bind(to:input:) #227

nflahavan opened this issue Jan 24, 2021 · 5 comments

Comments

@nflahavan
Copy link
Contributor

Like the title says there is a retain cycle in the various bind(to:input:) functions. The following test fails:

it("does not create a retain cycle") {
  var subject: UIBarButtonItem? = UIBarButtonItem(barButtonSystemItem: .save, target: nil, action: nil)
  let action = Action<String, String>(workFactory: { _ in
      return .empty()
  })

  subject?.rx.bind(to: action, input: "Hi there!")

  weak var subjectWeakReference = subject
  subject = nil

  expect(subjectWeakReference).to(beNil()) // fails!
}

The retain cycle is due to passing base to inputTransform in map. This can be fixed by changing the various bind methods to the following:

self.tap // or controlEvent
    .withUnretained(self.base) // no longer retaining base!
    .map(\.0)
    .map(inputTransform)
    .bind(to: action.inputs)
    .disposed(by: self.base.actionDisposeBag)

With the change above all tests plus the one I added to check for a retain cycle pass. I'd be happy to write up a pull request to make these changes if you agree they need to be made.

P.s. Thanks for the great framework!

@ashfurrow
Copy link
Member

Hi @nflahavan! Thank you for the detailed issue – the unit test is very helpful. Definitely a reference cycle here would be a bug. I would be happy to review a PR, thank you again.

@nflahavan
Copy link
Contributor Author

#228 is available for your review whenever you get a chance!

@Marcin951
Copy link

Marcin951 commented Jul 12, 2021

Hey @ashfurrow @nflahavan the issue is still not fixed. I'm using the Action version from the master, and still I can detect memory leaks

This simple code generates memory leak

    let forgotPasswordAction = CocoaAction {
        return .create { [weak self] (observer) -> Disposable in
            guard let self = self else { return Disposables.create() }
            self.delegate?.didTapForgotPasswordButton()
            
            observer.onCompleted()
            return Disposables.create()
        }
    }
    
    forgotPasswordButton.rx.action = forgotPasswordAction

@Marcin951
Copy link

#235 It's very similar but for button.rx.action case

@ashfurrow
Copy link
Member

Hi, thanks for following up and opening #235. I honestly don't use Action or RxSwift day-to-day anymore, so I don't have a lot of time to devote to fixing bugs like this. I'd be happy to review a pull request, though.

I also just noticed that #228 was merged but hasn't been released yet – let's wait until this further retain cycle is fixed and then release a version 4.3.1 👍

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

3 participants