Fix concurency issue on dissmis

[moglistree]

Moving the solution of this issue here.

There is a test that illustrates the problem attached to this PR. There where quite a few discussion in the other PR and the conclusion was that this is a Presentation issue and it should be fixed there.

The main issues is that in UIViewController+Presentation.swift there is a disposer that captures the completion block. This disposer gets triggered when the resulting future gets replaced with the .success() override and it does not happened on the main thread. This results in 2 issues:

1. There is an arbitrary delay between the presentation and the expected automatic dismiss expected from the .success().
2. The accessor sometimes tries to read the title property and its not always on the main thread.

This fix addresses both

[enhorn]

Is it required for present() to be called from main thread? Other wise we might want to ser the result schedular as a parameter with Scheduler.current as default value?

[moglistree]

Is it required for present() to be called from main thread? Other wise we might want to ser the result schedular as a parameter with Scheduler.current as default value?

I think @mansbernhardt has more info on this. I updated the description to explain why the result should happened on the same thread as the call.

[moglistree]

I can understand the crash caused by accessing the vc.title from the log calls. But I can't see what is causing the delay you are mentioning. Could you try to explain why this would happen?

Please refer to my answer on the log comment u have.

But if u run the test it will hit this issue as well if the log is commented out on the thread sanitiser.

[mansbernhardt]

I think this PR seem to work around an issue with the loggin of the title and instead should address that part. I suggest changing to something like this instead:

            let vcDescription = vc.presentationDescription
            let selfDescription = self.presentationDescription

return Future { futureCompletion in
            let bag = DisposeBag()

            let responder = self.restoreFirstResponder(options)

            let vcDescription = vc.presentationDescription
            let selfDescription = self.presentationDescription

            let onDismissedBag = DisposeBag()
            onDismissedBag += responder

            var didComplete = false
            var dismiss = { Future() }
            func completion(_ result: Result<Value>) {
                guard !didComplete else { return }
                didComplete = true

                let complete = {
                    log(.didDismiss(.init(vcDescription), from: .init(selfDescription), result: result.map { $0 as Any }))
                    futureCompletion(result)
                }

                if options.contains(.dontWaitForDismissAnimation) {
                    dismiss().always(onDismissedBag.dispose)
                    complete()
                } else {
                    dismiss().always(onDismissedBag.dispose).always(complete)
                }
            }

            bag += {
                    guard !didComplete else { return }
                    log(.didCancel(.init(vcDescription), from: .init(selfDescription)))
                    completion(.failure(PresentError.dismissed))
            }

As for the thread sanitiser, where does it report a race condition? In accessing the title? Or in the loggin code? If the former, that above should fix this. If the latter I think the log should either be updated to handle concurrent access or be updated to schedule all logging on the main thread. I would suggest the former (as typical logger should handle concurrent access)

[moglistree]

@mansbernhardt

I think the title issue is a bonus of addressing the main issue here. And if u look at the test it describes the main issue very precisely. We have a presentation that is modally presented and the result Future gets mutated with the succeed(with value: Value, after timeout: TimeInterval) -> Future operator that calls down to replace(with result: Result<Value>, after timeout: TimeInterval) -> Future operator. This results with a new Future. The replace operator uses the following code to trigger the work after the time give:

return disposableAsync(after: timeout) {
    future.cancel()
    completion(result)
}

This is where the issue with the race condition occurs since when the future gets canceled after the proposed time and the following disposer gets executed:

bag += {
    originalSheduler.async {
        guard !didComplete else { return }
        log(.didCancel(.init(vc.presentationDescription), from: .init(self.presentationDescription)))
        completion(.failure(PresentError.dismissed))
    }
}

This in-turn tries to dismiss the presented UIViewController and the sanitiser fires. This is since disposableAsync is schedules its work on concurrentBackground Scheduler. Switching back to the main thread in the disposer fixes this and the test illustrates it.