Skip to content

Commit

Permalink
[fix]: resolve memory exclusivity violations in some WorkflowObserver…
Browse files Browse the repository at this point in the history
… paths (#216)

### Issue

- under some circumstances, the action application observation codepath could trigger runtime memory exclusivity exceptions

### Description

- added a test case that reproduces the issue with the existing API
- removed an unneeded `inout` parameter to resolve the problem
  • Loading branch information
jamieQ authored May 22, 2023
1 parent dd9042c commit 51429c6
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 4 deletions.
4 changes: 0 additions & 4 deletions Workflow/Sources/WorkflowNode.swift
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ final class WorkflowNode<WorkflowType: Workflow> {
/// allowing the underlying conformance to be applied to the Workflow's State
let outputEvent = openAndApply(
action,
to: &state,
isExternal: source == .external
)

Expand Down Expand Up @@ -182,13 +181,10 @@ private extension WorkflowNode {
/// Applies an appropriate `WorkflowAction` to advance the underlying Workflow `State`
/// - Parameters:
/// - action: The `WorkflowAction` to apply
/// - state: The `State` to which the action will be applied
/// - observerInfo: Optional observation info to notify registered `WorkflowObserver`s
/// - isExternal: Whether the handled action came from the 'outside world' vs being bubbled up from a child node
/// - Returns: An optional `Output` produced by the action application
func openAndApply<A: WorkflowAction>(
_ action: A,
to state: inout WorkflowType.State,
isExternal: Bool
) -> WorkflowType.Output? where A.WorkflowType == WorkflowType {
let output: WorkflowType.Output?
Expand Down
58 changes: 58 additions & 0 deletions Workflow/Tests/WorkflowObserverTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,35 @@ final class WorkflowObserverTests: XCTestCase {
XCTAssertEqual(actions, [.toggle])
}

// added to exercise a simultaneous memory access bug when observing action
// application for some Workflow's where State == Void
func test_didReceiveActionCallbacks_voidState() {
var actions: [VoidStateWorkflow.Action] = []
observer.onDidReceiveAction = { action, workflow, session in
guard let action = action as? VoidStateWorkflow.Action else {
XCTFail("unexpected action. expecting \(VoidStateWorkflow.Action.self), got \(type(of: action))")
return
}

actions.append(action)
}

let node = WorkflowNode(
workflow: VoidStateWorkflow(),
parentSession: nil,
observer: observer
)

let rendering = node.render()
node.enableEvents()

XCTAssertEqual(actions, [])

rendering.onTapped()

XCTAssertEqual(actions, [.actionValue])
}

func test_didReceiveActionCallbacks_onlyInvokedForExternalEvents() {
var actionsReceived: [InjectableWorkflow.Action] = []
observer.onDidReceiveAction = { action, workflow, session in
Expand Down Expand Up @@ -687,3 +716,32 @@ private struct DefaultObservers: ObserversInterceptor {
observers + initialObservers
}
}

// MARK: Void State Observation Crash Example

/// Example that cause a memory exclusivity violation in prior observation code
private struct VoidStateWorkflow: Workflow {
typealias State = Void
typealias Output = Never

enum Action: WorkflowAction {
typealias WorkflowType = VoidStateWorkflow

case actionValue

func apply(toState state: inout VoidStateWorkflow.State) -> VoidStateWorkflow.Output? {
return nil
}
}

struct Rendering {
var onTapped: () -> Void
}

func render(state: VoidStateWorkflow.State, context: RenderContext<VoidStateWorkflow>) -> Rendering {
let sink = context.makeSink(of: VoidStateWorkflow.Action.self)
return Rendering(
onTapped: { sink.send(.actionValue) }
)
}
}

0 comments on commit 51429c6

Please sign in to comment.