CancelBag Strong Reference Cycle

[Sundea]

Binding is a struct that has two properties get and set which are closures (my assumption as far as they are marked in init as escaping). That means that it retains its value. Looking at your interactors I have found that sinkToLoadable retains the cancelBag, associated value of the loading case. So, that means that the publisher will never be canceled if cancelBag receives dealloc message. For example, if the interactor's method will be called again before the previous publisher finishes. So settings any other new value to binding to replace the previous loading(_, cancelBag) will not stop the publisher.
Does it was the desired behavior?

[Sundea]

@State var loadable: Loadable<Int> = .notRequested

var body: some View {
 VStack {
        Button("action") {
          let cancelBag: CancelBag = .init()
          let binding = Binding(get: {
            return loadable // captures the cancelBag
          }, set: { value in
            loadable = value
          })
          binding.wrappedValue.setIsLoading(cancelBag: cancelBag)
          print(binding.wrappedValue)
          let publisher = Just<Int>(1).delay(for: .seconds(5), scheduler: RunLoop.main)
          publisher
            .handleEvents(receiveCancel: {
              print("canceled")
            })
            .sink(receiveValue: { [binding] value in
              print("value received")
              print(binding)
              $loadable2.wrappedValue = .loaded(value)
            })
            .store(in: cancelBag)
        }
        Button("cancel") {
          loadable = .isLoading(last: nil, cancelBag: .init())
        }
      }
}

If u press button action several times it would not cancel previous requests

[nalexn]

Hey! I believe if you remove the binding from your sample code, the reference cycle should go away. It doesn't seem to be used for any state management in your code, just store the cancelBag in the view's loadable variable.

---

Oh, I see you're referring to a similar structure in the codebase of the project. Thanks for the heads up, this might actually be an issue. I'll have a look when I get time!

[Sundea]

@nalexn Thanks for your response!
It is an initial way how we update the interactor so binding is required. I've tried to hardcode the binging's get to .notRequsted and the problem goes away. But in this case, there we have a limitation for the getter, and pay attention to reading a value from it in interactors.

[Sundea]

My raw reimplementation of binding works perfectly instead of SwiftUI one's

struct XBinding<Value> {
    var wrappedValue: Value {
        get { return getValue() }
        nonmutating set { setValue(newValue) }
    }

    private let getValue: () -> Value
    private let setValue: (Value) -> Void

    init(getValue: @escaping () -> Value, setValue: @escaping (Value) -> Void) {
        self.getValue = getValue
        self.setValue = setValue
    }

    var projectedValue: Self { self }
}

This means that somewhere under the hood SwiftUI binding retains the value twice

[Sundea]

I have found a similar problem while writing directly to appState. The root cause differs, so even could suggest a fix for it.
So, the problem is following lines of code

extension Store {

  subscript<T>(keyPath: WritableKeyPath<Output, T>) -> T where T: Equatable {
    get { value[keyPath: keyPath] }
    set {
      var value = self.value
      if value[keyPath: keyPath] != newValue {
        value[keyPath: keyPath] = newValue
        self.value = value
      }
    }
  }

Imagine u have some loadable in appState and it equals to isLoading(_, _). If u will try to call appState[\.someloadable].setIsLoading(CancelBag()) it would do nothing. The If operator in a subscript above will not pass value to change cause Loadable's Eqautable implementation ignores it

extension Loadable: Equatable where T: Equatable {

  public static func == (lhs: Loadable<T>, rhs: Loadable<T>) -> Bool {
    let result: Bool
    switch (lhs, rhs) {
    case (.notRequested, .notRequested):
      result = true
    case (.isLoading(let lhsV, _), .isLoading(let rhsV, _)):
      result = lhsV == rhsV // the problem is here
    case (.loaded(let lhsV), .loaded(let rhsV)):
      result = lhsV == rhsV
    case (.failed(let lhsE), .failed(let rhsE)):
      result = lhsE.localizedDescription == rhsE.localizedDescription
    default:
      result = false
    }
    return result
  }
}

if the last value is equal, the old operation will never be canceled. Cause isLoading(last, oldCancelBag) == isLoading(last, newCancelBag) return true.
A suggestion is to compare the cancelBag as well in Equitable implementation by reference (===). Comparing by usual equal == will not help.

However, this approach will brake the tests, cause it XCTAssertsEqual will check for cancelBag as well. But this solutions could be solved by custom asserts

[Sundea]

Hi @nalexn!
Any updates?
After a few days of brainstorming, I've found only some workarounds, but not a proper fix. So, maybe you have some ideas, cause this is going to be a significant issue.

[nalexn]

Hey - sorry for a slow response. Today I reviewed the existing code, so here are my thoughts.
First of, I think your last suggestion for comparing the cancelbags in Loadable: Equatable is valid, I'll update the code. Some tests fail indeed.

Now about the original concern about cancelBag being retained. In RxSwift we're relying on the disposeBag's deallocation for canceling the subscriptions, so retaining it anywhere except in the owning object is indeed a problem leading to subscription leaking.

A peculiarity of SwiftUI is that almost everything is a struct, so you cannot use the same approach as with RxSwift due to the absence of "owning objects" limiting the lifetime of the subscriptions.

CancelBags are built differently than DisposeBags. They do cancel subscriptions upon deallocation, but they also offer the method cancel(), allowing you to release subscriptions while the bag is still retained.

Loadable enum also offers cancelLoading helper method that calls cancel() if it currently holds a cancelBag.

So, to the solution: if you have an event in your app when you want to stop all running requests (for example, when the user logs out) - you can call cancelLoading on all Loadables you have in the AppState. Alternatively, just reset the AppState to some default state, so all retained cancelBags should get deallocated (except if you still show a view that retains cancelbag through binging - then manual cancelLoading should kill the requests for sure)