RxSwiftCommunity / Action

Abstracts actions to be performed in RxSwift.
MIT License
875 stars 150 forks source link

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

Open nflahavan opened 3 years ago

nflahavan commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

https://github.com/RxSwiftCommunity/Action/pull/228 is available for your review whenever you get a chance!

Marcin951 commented 3 years ago

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 commented 3 years ago

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

ashfurrow commented 3 years ago

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 👍