[fix]: drop invalidated sink events instead of crashing (#234)

### Issue

the issue manifests in the following scenario:

1. an action is sent to node A
1. the subsequent re-render caused by handling the action removes node A from the tree, invalidating its EventPipe in the process
1. an observer of the WorkflowHost's rendering signal synchronously sends another event that propagates to node A when the rendering signal updates
1. action handling of this second event hits a fatal assertion because A’s event pipe has been marked as ‘invalid’

### Changes

- removes the fatal assertion in favor of dropping the events and emitting debug console logging/regular assertions when this occurs
- adds a reentrancy check to differentiate when a sink is being used multiple times in the same call stack, likely due to UIKit doing synchronous things when Renderings are being updated but before action handling has fully unwound from the stack
- adds a regular assertion failure in the case an invalidated Sink is messaged when not in the re-entrant scenario
- updates tests

i asked around, and it seems that the Kotlin runtime has not enforced this hard assertion for some time, and i could not think of a compelling reason to continue doing it.

Author: jamieQ

Workflow/Sources/SubtreeManager.swift

@@ -92,7 +92,7 @@ extension WorkflowNode {
         }
 
         /// Enable the eventPipes for the previous rendering. The eventPipes are not valid until this has
-        /// be called. If is an error to call this twice without generating a new rendering.
+        /// be called. It is an error to call this twice without generating a new rendering.
         func enableEvents() {
             /// Enable all action pipes.
             for eventPipe in eventPipes {
@@ -331,13 +331,20 @@ extension WorkflowNode.SubtreeManager {
             case invalid
         }
 
+        /// Utility to detect reentrancy in `handle()`
+        private var isHandlingEvent: Bool = false
+
         init() {
             self.validationState = .preparing
         }
 
         func handle(event: Output) {
             dispatchPrecondition(condition: .onQueue(DispatchQueue.workflowExecution))
 
+            let isReentrantCall = isHandlingEvent
+            isHandlingEvent = true
+            defer { isHandlingEvent = isReentrantCall }
+
             switch validationState {
             case .preparing:
                 fatalError("[\(WorkflowType.self)] Sink sent an action inside `render`. Sinks are not valid until `render` has completed.")
@@ -349,7 +356,26 @@ extension WorkflowNode.SubtreeManager {
                 handler(event)
 
             case .invalid:
-                fatalError("[\(WorkflowType.self)] Sink sent an action after it was invalidated. Sinks can only be used for a single valid `Rendering`.")
+                #if DEBUG
+                // Reentrancy seems to often be due to UIKit behaviors over
+                // which we have little control (e.g. synchronous resignation
+                // of first responder after a new Rendering is assigned). Emit
+                // some debug info in these cases.
+                if isReentrantCall {
+                    print("[\(WorkflowType.self)]: ℹ️ Sink sent another action after it was invalidated but before its original action handling was resolved. This new action will be ignored. If this is unexpected, set a Swift error breakpoint on `\(InvalidSinkSentAction.self)` to debug.")
+                }
+
+                do {
+                    throw InvalidSinkSentAction()
+                } catch {}
+                #endif
+
+                // If we're invalid and this is the first time `handle()` has
+                // been called, then it's likely we've somehow been inadvertently
+                // retained from the 'outside world'. Fail more loudly in this case.
+                assert(isReentrantCall, """
+                    [\(WorkflowType.self)]: Sink sent an action after it was invalidated. This action will be ignored.
+                """)
             }
         }
 
@@ -499,3 +525,9 @@ extension WorkflowNode.SubtreeManager {
         }
     }
 }
+
+// MARK: - Debugging Utilities
+
+#if DEBUG
+private struct InvalidSinkSentAction: Error {}
+#endif

Workflow/Tests/ConcurrencyTests.swift

@@ -160,9 +160,11 @@ final class ConcurrencyTests: XCTestCase {
         let secondScreen = host.rendering.value
         XCTAssertEqual(1, secondScreen.count)
 
-        // MANUAL TEST CASE: Uncomment to validate this fatal errors.
-        // Calling `update` uses the original sink. This will fail with a fatalError as the sink was not redeclared.
-        // initialScreen.update()
+        // Calling `update` uses the original sink. Historically this would be expected
+        // to trigger a fatal error, but as of https://github.com/square/workflow-swift/pull/189
+        // the internal event handling infrastructure is expected to have been
+        // torn down by this point, so this should just no-op.
+        initialScreen.update()
 
         // If the sink *was* still valid, this would be correct. However, it should just fail and be `1` still.
         // XCTAssertEqual(2, secondScreen.count)

Workflow/Tests/WorkflowHostTests.swift

@@ -52,3 +52,107 @@ final class WorkflowHostTests: XCTestCase {
         }
     }
 }
+
+// MARK: Event Emission Tests
+
+final class WorkflowHost_EventEmissionTests: XCTestCase {
+    // Previous versions of Workflow would fatalError under this scenario
+    func test_event_sent_to_invalidated_sink_during_action_handling() {
+        let root = Parent()
+        let host = WorkflowHost(workflow: root)
+        let initialRendering = host.rendering.value
+        var observedRenderCount = 0
+
+        XCTAssertEqual(initialRendering.eventCount, 0)
+
+        let disposable = host.rendering.signal.observeValues { rendering in
+            XCTAssertEqual(rendering.eventCount, 1)
+
+            // emit another event using an old rendering
+            // while the first is still being processed, but
+            // the workflow that handles the event has been
+            // removed from the tree
+            if observedRenderCount == 0 {
+                initialRendering.eventHandler()
+            }
+
+            observedRenderCount += 1
+        }
+        defer { disposable?.dispose() }
+
+        // send an event and cause a re-render
+        initialRendering.eventHandler()
+
+        XCTAssertEqual(observedRenderCount, 1)
+    }
+}
+
+// MARK: Utility Types
+
+extension WorkflowHost_EventEmissionTests {
+    struct Parent: Workflow {
+        struct Rendering {
+            var eventCount = 0
+            var eventHandler: () -> Void
+        }
+
+        typealias Output = Never
+
+        struct State {
+            var renderFirst = true
+            var eventCount = 0
+        }
+
+        func makeInitialState() -> State { .init() }
+
+        func render(state: State, context: RenderContext<Parent>) -> Rendering {
+            // swap which child is rendered
+            let key = state.renderFirst ? "first" : "second"
+            let handler = Child()
+                .mapOutput { _ in
+                    ParentAction.childChanged
+                }
+                .rendered(in: context, key: key)
+
+            return Rendering(
+                eventCount: state.eventCount,
+                eventHandler: handler
+            )
+        }
+
+        enum ParentAction: WorkflowAction {
+            typealias WorkflowType = Parent
+
+            case childChanged
+
+            func apply(toState state: inout Parent.State) -> Never? {
+                state.eventCount += 1
+                state.renderFirst.toggle()
+                return nil
+            }
+        }
+    }
+
+    struct Child: Workflow {
+        typealias Rendering = () -> Void
+        typealias State = Void
+        enum Output {
+            case eventOccurred
+        }
+
+        func render(state: Void, context: RenderContext<Child>) -> () -> Void {
+            let sink = context.makeSink(of: Action.self)
+            return { sink.send(Action.eventOccurred) }
+        }
+
+        enum Action: WorkflowAction {
+            typealias WorkflowType = Child
+
+            case eventOccurred
+
+            func apply(toState state: inout Void) -> Child.Output? {
+                return .eventOccurred
+            }
+        }
+    }
+}