wwt / SwiftCurrent

A library for managing complex workflows in Swift
https://wwt.github.io/SwiftCurrent/
Apache License 2.0
308 stars 19 forks source link

`_abandon` on `Workflow` not actually called. #41

Closed Tyler-Keith-Thompson closed 2 years ago

Tyler-Keith-Thompson commented 3 years ago

Describe the bug

There's a deceptive bit of code that looks like it functions, but in real world conditions is silently failing.

To Reproduce

Steps to reproduce the behavior:

  1. Put a breakpoint on the _abandon function in Workflow.
  2. Abandon a workflow with a UIKit presenter.
  3. Notice how your breakpoint didn't get triggered in real world conditions? (tests may lie a bit, depending)

Expected behavior

_abandon should have been called

Additional context

A quick and dirty fix, that I haven't investigated enough yet:

extension AnyWorkflow {
    /**docs...*/
    public func abandon(animated: Bool = true, onFinish:(() -> Void)? = nil) {
        let abandonRef = _abandon // Use this in all code that called self?._abandon() since self had been de-initialized by the time the closure executes.
        if let presenter = orchestrationResponder as? UIKitPresenter {
            presenter.abandon(self, animated: animated) { [weak self] in
                // self?._abandon() // LIES! self is NIL HERE
                abandonRef()
                onFinish?()
            }
        }
        // ... more code is here
    }
}
Tyler-Keith-Thompson commented 3 years ago

NOTE: This is likely a low priority bug, as behavior on the user side is unaffected, #39 cleans up a fair amount of the references that _abandon() was anyways. It may be that it's not longer even necessary to call _abandon at all once that gets merged in. It'll take a little more investigation to say for sure.

Regardless, this poses no serious issues with the library.

nickkaczmarek commented 2 years ago

@Tyler-Keith-Thompson I think this is working now. I checked out main and set breakpoints in the places you mentioned and all of them got hit. I'm going to close this and you can reopen if you observe the behavior.

nickkaczmarek commented 2 years ago

Temporarily reopening while I make some sample apps and try to see if I can break this as mentioned in the exposition.

nickkaczmarek commented 2 years ago

Found when this happens! If you call abandon in the onFinish completion handler, self is nil.

launchInto(workflow, args: "Noble Six", withLaunchStyle: .navigationStack) { passedArgs in // SwiftCurrent
    workflow.abandon()

    guard case .args(let emailAddress as String) = passedArgs else {
        print("No email address supplied")
        return
    }
    print(emailAddress)
}
nickkaczmarek commented 2 years ago

Found a fix for the bug. We just make our [weak self] strongly referenced because we need to hold onto that reference until we abandon. I want to do a little more testing and look into instrumenting this to prove that we've solved the memory leak.