Memory Leak in .publisher Effect when not using Navigation

[wshamp]

So I posted this originally thinking it was related to Shared State but I can repro using combine publishers so bringing it here. It seems when return a .publisher effect from onTask when the publisher was created at root and is retained there, if you swap views or nil out the state of the child without using Navigation we get a memory leak and purple warning. Maybe I am just setting up root incorrectly, any advice would be very helpful as this seems to be a common way to set up root with pre and post auth.

@Reducer
struct FeatureOne {
    @ObservableState
    struct State {
        let testPublisher: AnyPublisher<String, Never>
    }
    enum Action {
        case onTask
        case onDisappear
        case publishedValue
    }
    private enum CancelID {
        case cancelTestPublisher
    }
    var body : some ReducerOf<Self> {
        Reduce { state, action in
            switch action {
            case .onTask:
                return .publisher { [testPublisher = state.testPublisher] in
                                     testPublisher.map{_ in Action.publishedValue}
                }
                .cancellable(id: CancelID.cancelTestPublisher, cancelInFlight: true)
            case .publishedValue:
                return .none
            case .onDisappear:
                return .cancel(id: CancelID.cancelTestPublisher)
            }
        }
    }
}

struct FeatureOneView: View {
    let store: StoreOf<FeatureOne>
    var body: some View {
        Text("test")
            .onAppear {store.send(.onTask)}
            //.onDisappear {store.send(.onDisappear)}
            //.task { store.send(.onTask) }
            //.task { await store.send(.onTask).finish() }
    }
}

@Reducer
struct TestRootFeature {
    @ObservableState
    struct State {
        let testPublisher = CurrentValueSubject<String, Never>("test")
        var featureOne: FeatureOne.State? = nil
    }
    enum Action {
        case featureOne(FeatureOne.Action)
        case showButtonTapped
        case hideButtonTapped
    }
    var body: some ReducerOf<Self> {
        Reduce { state, action in
            switch action {
            case .hideButtonTapped:
                state.featureOne = nil
                state.testPublisher.send("Test") // *** This causes leak and warnings ***
                return .none
            case .showButtonTapped:
                state.featureOne = FeatureOne.State(testPublisher: state.testPublisher.eraseToAnyPublisher())
                return .none
            case .featureOne:
                return .none
            }
        }
        .ifLet(\.featureOne, action: \.featureOne) {
            FeatureOne()
        }
    }
}

struct TestRootView: View {
    let store: StoreOf<TestRootFeature>
    var body: some View {
        VStack {
            if let featureStore = store.scope(state: \.featureOne, action: \.featureOne) {
                VStack {
                    FeatureOneView(store: featureStore)
                    Button {
                        store.send(.hideButtonTapped)
                    } label: {
                        Text("Hide")
                    }
                }
            } else {
                Button {
                    store.send(.showButtonTapped)
                } label: {
                    Text("Show")
                }
            }
        }
    }
}

[mbrandonw]

Hi @wshamp, this is functioning as is to be expected, but there are a few strange things about your sample that we highly encourage you to change.

First, the effect is definitely being cancelled, but it is not canceled until the ifLet operator has fully run its logic. It is not cancelled the immediate moment you do featureOne = nil. So, when you do this:

state.featureOne = nil
state.testPublisher.send("Test")

…the effect in the child sees the emitted "Test" and sends an action back into the store, and at that moment the effect is not cancelled. It isn't until the end of the reducer that the effect is actually torn down. Really there is no choice but for it to behave this way.

However, it is not appropriate to hold onto a publisher directly in state. State is meant to be just the simple data your feature needs to do its job. Things like publishers and other effects go in dependencies. And further, you should not be interact with outside systems (like subjects or dependencies in general) directly in the reducer, but rather that should go in an effect.

So, the more correct way of implementing this demo is something like this:

@Reducer
struct FeatureOne {
  @ObservableState
  struct State {
  }
  enum Action {
    case onAppear
    case publishedValue
  }
  @Dependency(TestPublisher.self) var testPublisher
  var body : some ReducerOf<Self> {
    Reduce { state, action in
      switch action {
      case .onAppear:
        return .publisher {
          testPublisher.publisher()
            .map { _ in .publishedValue }
        }
      case .publishedValue:
        return .none
      }
    }
  }
}

struct FeatureOneView: View {
  let store: StoreOf<FeatureOne>
  var body: some View {
    Text("test")
      .onAppear { store.send(.onAppear) }
  }
}

@Reducer
struct TestRootFeature {
  @ObservableState
  struct State {
    var featureOne: FeatureOne.State? = nil
  }
  enum Action {
    case featureOne(FeatureOne.Action)
    case showButtonTapped
    case hideButtonTapped
  }
  @Dependency(TestPublisher.self) var testPublisher
  var body: some ReducerOf<Self> {
    Reduce { state, action in
      switch action {
      case .hideButtonTapped:
        state.featureOne = nil
        //state.testPublisher.send("Test")
        return .run { _ in
          await testPublisher.send("Test")
        }
      case .showButtonTapped:
        state.featureOne = FeatureOne.State()
        return .none
      case .featureOne:
        return .none
      }
    }
    .ifLet(\.featureOne, action: \.featureOne) {
      FeatureOne()
    }
  }
}

struct TestRootView: View {
  let store: StoreOf<TestRootFeature>
  var body: some View {
    VStack {
      if let store = store.scope(state: \.featureOne, action: \.featureOne) {
        FeatureOneView(store: store)
      }
      HStack {
        Button {
          store.send(.showButtonTapped)
        } label: {
          Text("Show")
        }
        Button {
          store.send(.hideButtonTapped)
        } label: {
          Text("Hide")
        }
      }
    }
  }
}

@preconcurrency import Combine
struct TestPublisher: DependencyKey {
  var get: () -> String
  var send: (String) async -> Void
  var publisher: () -> AnyPublisher<String, Never>

  static var liveValue: TestPublisher {
    let subject = CurrentValueSubject<String, Never>("")
    return TestPublisher(
      get: { subject.value },
      send: { subject.send($0) },
      publisher: { subject.eraseToAnyPublisher() }
    )
  }
}

Notice that there is no publisher held directly in state, and further there is no need for explicit cancellation. It is handled automatically by the ifLet tool. I also think that the new @Shared property wrapper would suite your needs better here, but that is currently in beta, though it will be released soon.

[wshamp]

So I was just using publishers to show this happening outside of SharedState as I found this issue using the beta and I posted about that separately. So if you just replace the combine publisher with a @shared property you will have the same issue. Aren't we technically holding publishers in state with shared state properties as you can observe their changes using their publisher property?

https://github.com/pointfreeco/swift-composable-architecture/discussions/2857#discussioncomment-9167119

[mbrandonw]

Closing this in favor of keeping the discussion in #2857 (comment).