Dispose viewConnectable synchronously (#190)

MobiusCore/Source/AsyncDispatchQueueConnectable.swift

@@ -14,24 +14,15 @@
 
 import Foundation
 
-/// A connectable adapter which imposes asynchronous dispatch blocks around calls to `accept` and `dispose`.
+/// A connectable adapter which imposes asynchronous dispatch blocks around calls to `accept`.
 ///
-/// Creates `Connection`s that forward invocations to `accept` and `dispose` to a connection returned by the underlying
-/// connectable, first switching to the provided `acceptQueue`. In other words, the real `accept` and `dispose` methods
-/// will always be executed asynchronously on the provided queue.
-///
-/// If the connection’s consumer is invoked between the Connectable’s `dispose` and the underlying asynchronous
-/// `dispose`, the call will not be forwarded.
+/// Creates `Connection`s that forward invocations to `accept` to a connection returned by the underlying connectable,
+/// first switching to the provided `acceptQueue`. In other words, the real `accept` method will always be executed
+/// asynchronously on the provided queue.
 final class AsyncDispatchQueueConnectable<Input, Output>: Connectable {
     private let underlyingConnectable: AnyConnectable<Input, Output>
     private let acceptQueue: DispatchQueue
 
-    private enum DisposalStatus: Equatable {
-        case notDisposed
-        case pendingDispose
-        case disposed
-    }
-
     init(
         _ underlyingConnectable: AnyConnectable<Input, Output>,
         acceptQueue: DispatchQueue
@@ -47,21 +38,16 @@ final class AsyncDispatchQueueConnectable<Input, Output>: Connectable {
         self.init(AnyConnectable(underlyingConnectable), acceptQueue: acceptQueue)
     }
 
-    func connect(_ consumer: @escaping (Output) -> Void) -> Connection<Input> {
-        let disposalStatus = Synchronized(value: DisposalStatus.notDisposed)
+    func connect(_ consumer: @escaping Consumer<Output>) -> Connection<Input> {
+        // A synchronized optional consumer allows for clearing the reference when it is no longer valid, which serves
+        // as the signal for the disposal status and also protects against state changes within critical regions.
+        let protectedConsumer = Synchronized<Consumer<Output>?>(value: consumer)
 
         let connection = underlyingConnectable.connect { value in
-            // Don’t forward if we’re currently waiting to dispose the connection.
-            //
-            // NOTE: the underlying consumer must be called inside the critical region accessing disposalStatus, or we
-            // could potentially enter the .pendingDispose state before or during the consumer call. This means that the
-            // underlying consumer must not call our connection’s acceptClosure or disposeClosure, or it will deadlock.
-            //
-            // This is OK in our existing use case because the underlying consumer is always flipEventsToLoopQueue from
-            // MobiusController’s initializer, which enters the actual Mobius loop asynchronously. If we exposed this
-            // class, it would be a scary edge case.
-            disposalStatus.read { status in
-                guard status != .pendingDispose else { return }
+            protectedConsumer.read { consumer in
+                guard let consumer = consumer else {
+                    MobiusHooks.errorHandler("cannot consume value after dispose", #file, #line)
+                }
                 consumer(value)
             }
         }
@@ -72,14 +58,13 @@ final class AsyncDispatchQueueConnectable<Input, Output>: Connectable {
                     connection.accept(input)
                 }
             },
-            disposeClosure: { [acceptQueue] in
-                guard disposalStatus.compareAndSwap(expected: .notDisposed, with: .pendingDispose) else {
-                    MobiusHooks.errorHandler("cannot dispose more than once", #file, #line)
-                }
-
-                acceptQueue.async {
+            disposeClosure: {
+                protectedConsumer.mutate { consumer in
+                    guard consumer != nil else {
+                        MobiusHooks.errorHandler("cannot dispose more than once", #file, #line)
+                    }
                     connection.dispose()
-                    disposalStatus.value = .disposed
+                    consumer = nil
                 }
             }
         )

MobiusCore/Source/Lock.swift

@@ -56,16 +56,3 @@ final class Synchronized<Value> {
         }
     }
 }
-
-extension Synchronized where Value: Equatable {
-    func compareAndSwap(expected: Value, with newValue: Value) -> Bool {
-        var success = false
-        self.mutate { value in
-            if value == expected {
-                value = newValue
-                success = true
-            }
-        }
-        return success
-    }
-}

MobiusCore/Source/MobiusController.swift

@@ -94,18 +94,13 @@ public final class MobiusController<Model, Event, Effect> {
         )
         self.state = state
 
-        // Maps an event consumer to a new event consumer that invokes the original one on the loop queue,
-        // asynchronously.
+        // Maps an event consumer to a new event consumer that asynchronously invokes the original on the loop queue.
         //
         // The input will be the core `MobiusLoop`’s event dispatcher, which asserts that it isn’t invoked after the
-        // loop is disposed. This doesn’t play nicely with asynchrony, so here we assert when the transformed event
-        // consumer is invoked, but fail silently if the controller is stopped before the asynchronous block executes.
+        // loop is disposed. This doesn’t play nicely with asynchrony, so here we fail silently if the controller is
+        // stopped before the asynchronous block executes.
         func flipEventsToLoopQueue(consumer: @escaping Consumer<Event>) -> Consumer<Event> {
             return { event in
-                guard state.running else {
-                    MobiusHooks.errorHandler("\(Self.debugTag): cannot accept events when stopped", #file, #line)
-                }
-
                 loopQueue.async {
                     guard state.running else {
                         // If we got here, the controller was stopped while this async block was queued. Callers can’t
@@ -211,9 +206,14 @@ public final class MobiusController<Model, Event, Effect> {
                 var disposables: [Disposable] = [loop]
 
                 if let viewConnectable = stoppedState.viewConnectable {
-                    let viewConnection = viewConnectable.connect { [unowned loop] event in
-                        // Note: loop.unguardedDispatchEvent will call our flipEventsToLoopQueue, which implements the
-                        //       assertion “unguarded” refers to, and also (of course) flips to the loop queue.
+                    let viewConnection = viewConnectable.connect { [weak loop] event in
+                        guard let loop = loop else {
+                            // This failure should not be reached under normal circumstances because it is handled by
+                            // AsyncDispatchQueueConnectable. Stopping here means that the viewConnectable called its
+                            // consumer reference after stop() has disposed the connection and deallocated the loop.
+                            MobiusHooks.errorHandler("\(Self.debugTag): cannot use invalid consumer", #file, #line)
+                        }
+
                         loop.unguardedDispatchEvent(event)
                     }
                     loop.addObserver(viewConnection.accept)
@@ -228,7 +228,7 @@ public final class MobiusController<Model, Event, Effect> {
             }
         } catch {
             MobiusHooks.errorHandler(
-                errorMessage(error, default: "\(Self.debugTag): cannot start a while already running"),
+                errorMessage(error, default: "\(Self.debugTag): cannot start a controller while already running"),
                 #file,
                 #line
             )

MobiusCore/Test/AsyncDispatchQueueConnectableTests.swift

@@ -0,0 +1,58 @@
+// Copyright 2019-2022 Spotify AB.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+@testable import MobiusCore
+
+import Foundation
+import Nimble
+import Quick
+
+class AsyncDispatchQueueConnectableTests: QuickSpec {
+    private let acceptQueue = DispatchQueue(label: "accept queue")
+
+    override func spec() {
+        describe("AsyncDispatchQueueConnectable") {
+            var connectable: AsyncDispatchQueueConnectable<String, String>!
+            var underlyingConnectable: RecordingTestConnectable!
+
+            beforeEach {
+                underlyingConnectable = RecordingTestConnectable(expectedQueue: self.acceptQueue)
+                connectable = AsyncDispatchQueueConnectable(underlyingConnectable, acceptQueue: self.acceptQueue)
+            }
+
+            context("when connected") {
+                var connection: Connection<String>!
+                var dispatchedValue: String?
+
+                beforeEach {
+                    connection = connectable.connect { dispatchedValue = $0 }
+                }
+
+                afterEach {
+                    dispatchedValue = nil
+                }
+
+                it("should forward inputs to the underlying connectable") {
+                    connection.accept("S")
+                    expect(underlyingConnectable.recorder.items).toEventually(equal(["S"]))
+                }
+
+                it("should forward outputs from the underlying connectable") {
+                    underlyingConnectable.dispatch("S")
+                    expect(dispatchedValue).to(equal("S"))
+                }
+            }
+        }
+    }
+}

MobiusCore/Test/MobiusControllerTests.swift

@@ -127,15 +127,6 @@ class MobiusControllerTests: QuickSpec {
                             controller.start()
                             expect(controller.running).to(beTrue())
                         }
-                        it("should ignore events sent while view disposal is pending") {
-                            self.viewQueue.sync {
-                                controller.stop()
-                                // Sending an event via the view connection here is valid, because the view connection has
-                                // not yet been disposed; that disposal is pending in a block enqueued on the view queue by
-                                // AsyncDispatchQueueConnectable’s asynchronous disposal block
-                                view.dispatchSameQueue("late event")
-                            }
-                        }
                     }
                 }
 
@@ -153,12 +144,12 @@ class MobiusControllerTests: QuickSpec {
                         controller.start()
                     }
 
-                    it("Should dispose any listeners of the model") {
+                    it("should dispose any listeners of the model") {
                         controller.stop()
                         expect(modelObserver.disposed).toEventually(beTrue())
                     }
 
-                    it("Should dispose any effect handlers") {
+                    it("should dispose any effect handlers") {
                         controller.stop()
                         expect(effectObserver.disposed).to(beTrue())
                     }

MobiusCore/Test/TestingUtil.swift

@@ -64,8 +64,6 @@ class RecordingTestConnectable: Connectable {
     }
 
     func dispose() {
-        verifyQueue()
-
         disposed = true
     }