Explicit effect disposables management (#39)

* fix(Store): Effect disposables do not deinitialize

* Merge branch 'effect-disposables-deinitialization' into main

* fix: ActionDisposable leaks

* fix: PR notes

* fix: PR notes

Author: maximkrouk

Sources/ComposableArchitecture/Store.swift

@@ -7,12 +7,16 @@ import ReactiveSwift
 /// You will typically construct a single one of these at the root of your application, and then use
 /// the `scope` method to derive more focused stores that can be passed to subviews.
 public final class Store<State, Action> {
-  @MutableProperty private(set) var state: State
+  @MutableProperty
+  private(set) var state: State
+  
   private var isSending = false
   private let reducer: (inout State, Action) -> Effect<Action, Never>
   private var synchronousActionsToSend: [Action] = []
   private var bufferedActions: [Action] = []
-
+  internal var effectDisposables: [UUID: Disposable] = [:]
+  internal var parentDisposable: Disposable?
+  
   /// Initializes a store from an initial state, a reducer, and an environment.
   ///
   /// - Parameters:
@@ -29,7 +33,7 @@ public final class Store<State, Action> {
       reducer: { reducer.run(&$0, $1, environment) }
     )
   }
-
+  
   /// Scopes the store to one that exposes local state and actions.
   ///
   /// This can be useful for deriving new stores to hand to child views in an application. For
@@ -170,11 +174,12 @@ public final class Store<State, Action> {
         return .none
       }
     )
-    self.$state.producer
-      .startWithValues { [weak localStore] newValue in localStore?.state = toLocalState(newValue) }
+    localStore.parentDisposable = self.$state.producer.startWithValues { [weak localStore] state in
+      localStore?.state = toLocalState(state)
+    }
     return localStore
   }
-
+  
   /// Scopes the store to one that exposes local state.
   ///
   /// - Parameter toLocalState: A function that transforms `State` into `LocalState`.
@@ -184,7 +189,7 @@ public final class Store<State, Action> {
   ) -> Store<LocalState, Action> {
     self.scope(state: toLocalState, action: { $0 })
   }
-
+  
   /// Scopes the store to a producer of stores of more local state and local actions.
   ///
   /// - Parameters:
@@ -196,14 +201,14 @@ public final class Store<State, Action> {
     state toLocalState: @escaping (Effect<State, Never>) -> Effect<LocalState, Never>,
     action fromLocalAction: @escaping (LocalAction) -> Action
   ) -> Effect<Store<LocalState, LocalAction>, Never> {
-
+    
     func extractLocalState(_ state: State) -> LocalState? {
       var localState: LocalState?
       _ = toLocalState(Effect(value: state))
         .startWithValues { localState = $0 }
       return localState
     }
-
+    
     return toLocalState(self.$state.producer)
       .map { localState in
         let localStore = Store<LocalState, LocalAction>(
@@ -212,17 +217,17 @@ public final class Store<State, Action> {
             self.send(fromLocalAction(localAction))
             localState = extractLocalState(self.state) ?? localState
             return .none
-          })
-
-        self.$state.producer
-          .startWithValues { [weak localStore] state in
-            guard let localStore = localStore else { return }
-            localStore.state = extractLocalState(state) ?? localStore.state
           }
+        )
+        
+        localStore.parentDisposable = self.$state.producer.startWithValues { [weak localStore] state in
+          guard let localStore = localStore else { return }
+          localStore.state = extractLocalState(state) ?? localStore.state
+        }
         return localStore
       }
   }
-
+  
   /// Scopes the store to a producer of stores of more local state and local actions.
   ///
   /// - Parameter toLocalState: A function that transforms a producer of `State` into a producer
@@ -234,66 +239,95 @@ public final class Store<State, Action> {
   ) -> Effect<Store<LocalState, Action>, Never> {
     self.producerScope(state: toLocalState, action: { $0 })
   }
-
+  
   func send(_ action: Action) {
     if !self.isSending {
       self.synchronousActionsToSend.append(action)
     } else {
       self.bufferedActions.append(action)
       return
     }
-
+    
     while !self.synchronousActionsToSend.isEmpty || !self.bufferedActions.isEmpty {
       let action =
         !self.synchronousActionsToSend.isEmpty
         ? self.synchronousActionsToSend.removeFirst()
         : self.bufferedActions.removeFirst()
-
+      
       self.isSending = true
       let effect = self.reducer(&self.state, action)
       self.isSending = false
-
+      
+      var didComplete = false
+      let effectID = UUID()
+      
       var isProcessingEffects = true
-      effect.startWithValues { [weak self] action in
-        if isProcessingEffects {
-          self?.synchronousActionsToSend.append(action)
-        } else {
-          self?.send(action)
+      
+      let observer = Signal<Action, Never>.Observer(
+        value: { [weak self] action in
+          if isProcessingEffects {
+            self?.synchronousActionsToSend.append(action)
+          } else {
+            self?.send(action)
+          }
+        },
+        failed: .none,
+        completed: { [weak self] in
+          didComplete = true
+          self?.effectDisposables.removeValue(forKey: effectID)?.dispose()
+        },
+        interrupted: { [weak self] in
+          didComplete = true
+          self?.effectDisposables.removeValue(forKey: effectID)?.dispose()
         }
-      }
+      )
+      let effectDisposable = effect.start(observer)
       isProcessingEffects = false
+      
+      if !didComplete {
+        self.effectDisposables[effectID] = effectDisposable
+      } else {
+        effectDisposable.dispose()
+      }
     }
   }
-
+  
   /// Returns a "stateless" store by erasing state to `Void`.
   public var stateless: Store<Void, Action> {
     self.scope(state: { _ in () })
   }
-
+  
   /// Returns an "actionless" store by erasing action to `Never`.
   public var actionless: Store<State, Never> {
     func absurd<A>(_ never: Never) -> A {}
     return self.scope(state: { $0 }, action: absurd)
   }
-
+  
   private init(
     initialState: State,
     reducer: @escaping (inout State, Action) -> Effect<Action, Never>
   ) {
     self.reducer = reducer
     self.state = initialState
   }
+  
+  deinit {
+    self.parentDisposable?.dispose()
+    self.effectDisposables.keys.forEach { id in
+      self.effectDisposables.removeValue(forKey: id)?.dispose()
+    }
+  }
 }
 
 /// A producer of store state.
 @dynamicMemberLookup
 public struct Produced<Value>: SignalProducerConvertible {
   public let producer: Effect<Value, Never>
-
+  
   init(by upstream: Effect<Value, Never>) {
     self.producer = upstream
   }
-
+  
   /// Returns the resulting producer of a given key path.
   public subscript<LocalValue>(
     dynamicMember keyPath: KeyPath<Value, LocalValue>
@@ -303,9 +337,9 @@ public struct Produced<Value>: SignalProducerConvertible {
 }
 
 @available(
-  *, deprecated,
-  message:
-    """
+*, deprecated,
+message:
+"""
 Consider using `Produced<State>` instead, this typealias is added for backward compatibility and will be removed in the next major release.
 """
 )

Sources/ComposableArchitecture/ViewStore.swift

@@ -68,6 +68,8 @@ public final class ViewStore<State, Action> {
     """
   )
   public var publisher: StoreProducer<State> { produced }
+  
+  internal var viewDisposable: Disposable?
 
   /// Initializes a view store from a store.
   ///
@@ -83,8 +85,8 @@ public final class ViewStore<State, Action> {
     self.produced = Produced(by: producer)
     self.state = store.state
     self._send = store.send
-    producer.startWithValues { [weak self] in
-      self?.state = $0
+    self.viewDisposable = producer.startWithValues { [weak self] state in
+      self?.state = state
     }
   }
 
@@ -257,6 +259,10 @@ public final class ViewStore<State, Action> {
       self.binding(send: { _ in action })
     }
   #endif
+  
+  deinit {
+    viewDisposable?.dispose()
+  }
 }
 
 extension ViewStore where State: Equatable {

Tests/ComposableArchitectureTests/StoreTests.swift

@@ -4,6 +4,39 @@ import XCTest
 @testable import ComposableArchitecture
 
 final class StoreTests: XCTestCase {
+  
+  func testEffectDisposablesDeinitialization() {
+    enum Action {
+      case triggerDelay
+      case delayDidComplete
+    }
+    let delayedReducer = Reducer<Void, Action, DateScheduler> { _, action, mainQueue in
+      switch action {
+      case .triggerDelay:
+        return Effect(value: .delayDidComplete).delay(1, on: mainQueue)
+        
+      case .delayDidComplete:
+        return .none
+      }
+    }
+
+    let store = Store(
+      initialState: (),
+      reducer: delayedReducer,
+      environment: QueueScheduler.main
+    )
+    
+    store.send(.triggerDelay)
+    store.send(.triggerDelay)
+    store.send(.triggerDelay)
+    store.send(.delayDidComplete)
+    
+    XCTAssertEqual(store.effectDisposables.count, 3)
+    
+    XCTWaiter().wait(for: [XCTestExpectation()], timeout: 1.1)
+    
+    XCTAssertEqual(store.effectDisposables.count, 0)
+  }
 
   func testScopedStoreReceivesUpdatesFromParent() {
     let counterReducer = Reducer<Int, Void, Void> { state, _, _ in