In Swift, if Thread.current.isMainThread == false, then is it safe to DispatchQueue.main.sync recursively once?

Question

In Swift, if Thread.current.isMainThread == false, then is it safe to DispatchQueue.main.sync recursively once?

The reason I ask is that, in my company's app, we had a crash that turned out to be due to some UI method being called from off the main thread, like:

public extension UIViewController {
    func presentModally(_ viewControllerToPresent: UIViewController, animated flag: Bool, completion: (() -> Void)? = nil) {
        // some code that sets presentation style then:
        present(viewControllerToPresent, animated: flag, completion: completion)
    }
}

Since this was getting called from many places, some of which would sometimes call it from a background thread, we were getting crashes here and there.

Fixing all the call sites was not feasible due to the app being over a million lines of code, so my solution to this was simply to check if we're on the main thread, and if not, then redirect the call to the main thread, like so:

public extension UIViewController {
    func presentModally(_ viewControllerToPresent: UIViewController, animated flag: Bool, completion: (() -> Void)? = nil) {
        guard Thread.current.isMainThread else {
            DispatchQueue.main.sync { 
                presentModally(viewControllerToPresent, animated: flag, completion: completion) 
            }
            return
        }

        // some code that sets presentation style then:
        present(viewControllerToPresent, animated: flag, completion: completion)
    }
}

The benefits of this approach seem to be:

• Preservation of execution order. If the caller is off the main thread, we'll redirect onto the main thread, then execute the same function before we return -- thus preserving the normal execution order that the would have happened had the original function been called from the main thread, since functions called on the main thread (or any other thread) execute synchronously by default.
• Ability to implicitly reference self without compiler warnings. In Xcode 11.4, performing this call synchronously also satisfies the compiler that it's OK to implicitly retain self, since the dispatch context will be entered then exited before the original function call returns -- so we don't get any new compiler warnings from this approach. That's nice and clean.
• More focused diffs via less indentation. It avoids wrapping the entire function body in a closure (like you'd normally see done if Dispatch.main.async { ... } was used, where the whole body must now be indented a level deeper, incurring whitespace diffs in your PR that can lead to annoying merge conflicts and make it harder for reviewers to distinguish the salient elements in GitHub's PR diff views).

Meanwhile the alternative, DispatchQueue.main.async, would seem to have the following drawbacks:

• Potentially changes expected execution order. The function would return before executing the dispatched closure, which in turn means that self could have deallocated before it runs. That means we'd have to explicitly retain self (or weakify it) to avoid a compiler warning. It also means that, in this example, present(...) would not get called before the function would return to the caller. This could cause the modal to pop-up after some other code subsequent to the call site, leading to unintended behavior.
• Requirement of either weakifying or explicitly retaining self. This is not really a drawback but it's not as clean, stylistically, as being able to implicitly retain self.

So the question is: are these assumptions all correct, or am I missing something here?

My colleagues who reviewed the PR seemed to feel that using "DispatchQueue.main.sync" is somehow inherently bad and risky, and could lead to a deadlock. While I realize that using this from the main thread would indeed deadlock, here we explicitly avoid that here using a guard statement to make sure we're NOT on the main thread first.

Despite being presented with all the above rationale, and despite being unable to explain to me how a deadlock could actually happen given that the dispatch only happens if the function gets called off the main thread to begin with, my colleagues still have deep reservations about this pattern, feeling that it could lead to a deadlock or block the UI in unexpected ways.

Are those fears founded? Or is this pattern perfectly safe?

Answer 1

A bit late, but I had a need for this type of solution too. I had some common code that could be invoked from both the main thread and background threads, and updated the UI. My solution to the generic use case was:

public extension UIViewController {
    func runOnUiThread(closure: @escaping () -> ()) {
        if Thread.isMainThread {
            closure()
        } else {
            DispatchQueue.main.sync(execute: closure)
        }
    }
}

Then to call it from a UIViewController:

runOnUiThread {
    code here
}

As others have pointed out, this is not completely safe. You might have some code on background thread that is invoked from the main thread, synchronously. If that background code then calls the code above, it will attempt to run on the main thread and will create a deadlock. The main thread is waiting for the background code to execute, and the background code will wait for the main thread to be free.

Answer 2

This pattern is definitely not “perfectly” safe. One can easily contrive a deadlock:

let group = DispatchGroup()
DispatchQueue.global().async(group: group) {
    self.presentModally(controller, animated: true)
}
group.wait()

Checking that isMainThread is false is insufficient, strictly speaking, to know whether it’s safe to dispatch synchronously to the main thread.

But that’s not the real issue. You obviously have some routine somewhere that thinks it’s running on the main thread, when it’s not. Personally, I’d be worried about what else that code did while operating under this misconception (e.g. unsynchronized model updates, etc.).

Your workaround, rather than fixing the root cause of the problem, is just hiding it. As a general rule, I would not suggest coding around bugs introduced elsewhere in the codebase. You really should just figure out where you’re calling this routine from a background thread and resolve that.

---

In terms of how to find the problem, hopefully the stack trace associated with the crash will tell you. I’d also suggest adding a breakpoint for the main thread checker by clicking on that little arrow next to it in the scheme settings:

Then exercise the app and if it encounters this issue, it will pause execution at the offending line, which can be very useful in tracking down these issues. That often is much easier than reverse-engineering from the stack trace.

Answer 3

I agree with the comments that you have some structural difficulties with your code.

But there are still times in which I need code to run on the main thread and I don't know if I'm already on the main thread or not. This has occurred often enough that I wrote a ExecuteOnMain() function just for this:

dispatch_queue_t MainSequentialQueue( )
{
    static dispatch_queue_t mainQueue;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{
#if HAS_MAIN_RUNLOOP
        // If this process has a main thread run loop, queue sequential tasks to run on the main thread
        mainQueue = dispatch_get_main_queue();
#else
        // If the process doesn't execute in a run loop, create a sequential utility thread to perform these tasks
        mainQueue = dispatch_queue_create("main-sequential",DISPATCH_QUEUE_SERIAL);
#endif
        });
    return mainQueue;
}

BOOL IsMainQueue( )
{
#if HAS_MAIN_RUNLOOP
    // Return YES if this code is already executing on the main thread
    return [NSThread isMainThread];
#else
    // Return YES if this code is already executing on the sequential queue, NO otherwise
    return ( MainSequentialQueue() == dispatch_get_current_queue() );
#endif
}

void DispatchOnMain( dispatch_block_t block )
{
    // Shorthand for asynchronously dispatching a block to execute on the main thread
    dispatch_async(MainSequentialQueue(),block);
}

void ExecuteOnMain( dispatch_block_t block )
{
    // Shorthand for synchronously executing a block on the main thread before returning.
    // Unlike dispatch_sync(), this won't deadlock if executed on the main thread.
    if (IsMainQueue())
        // If this is the main thread, execute the block immediately
        block();
    else
        // If this is not the main thread, queue the block to execute on the main queue and wait for it to finish
        dispatch_sync(MainSequentialQueue(),block);
}