Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_abandon on Workflow not actually called. #41

Closed
Tyler-Keith-Thompson opened this issue Jun 11, 2021 · 5 comments · Fixed by #190
Closed

_abandon on Workflow not actually called. #41

Tyler-Keith-Thompson opened this issue Jun 11, 2021 · 5 comments · Fixed by #190
Labels
bug Something isn't working low priority This is not a current focus

Comments

@Tyler-Keith-Thompson
Copy link
Collaborator

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 Tyler-Keith-Thompson added the bug Something isn't working label Jun 11, 2021
@Tyler-Keith-Thompson
Copy link
Collaborator Author

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.

@Richard-Gist Richard-Gist added the low priority This is not a current focus label Jun 14, 2021
@nickkaczmarek
Copy link
Contributor

@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
Copy link
Contributor

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

@nickkaczmarek nickkaczmarek reopened this Mar 3, 2022
@nickkaczmarek
Copy link
Contributor

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working low priority This is not a current focus
Projects
None yet
3 participants