Discussion: Safety of using NS(Recursive)Lock across awaits

[KaiOelfke]

Thank you for open sourcing this approach. I don't have a real issue, but wanted to share some thoughts. There's no discussions tab hence the issue.

I was curious to better understand how this works, because initially I assumed this might not be safe.

I thought locking across awaits is not safe, because locks have to be unlocked on the same thread.

"NSLock can be used safely but require caution to only use in synchronous code and not across an await."

Source: https://forums.swift.org/t/incremental-migration-to-structured-concurrency/54939/21

 public func wait() async {
        lock()
        // ...
        await withUnsafeContinuation { continuation in
            // ...
            unlock()
        }
    }

I wasn't sure if this could deadlock on small machine with a pool size of one, if at the suspension point marked by the await another async wait is run first and then trying to get the lock. Or if the closure would be executed on another thread as with Swift Concurrency there's not much guarantees about the thread on which an async function runs.

I now think this works, because of special behavior of withUnsafeContinuation:

"The closure is then immediately executed in the same execution context (in other words, the current thread) [...]"
Source: https://forums.swift.org/t/clarification-needed-on-unsafecontinuation-documentation/57803/2

If withTaskCancellationHandler and withUnsafeThrowingContinuation also have behave like that it starts to make more sense for me.

Or is there another reason why the locking code works in this case?

[groue]

Hi @KaiOelfke,

I migrated the issue into a discussion :-)

Thanks for opening this discussion. I am myself full of doubts, and can't pretend that the current implementation is the best possible one, or that it matches what was intended by the authors of Swift concurrency. Since the synchronization apis we need do not exist in the standard library, and that something has to be done, here we are.

I thought locking across awaits is not safe, because locks have to be unlocked on the same thread.

We're not changing thread.

The withUnsafeContinuation method accepts a closure that is documented to be immediately executed (as you have noticed). Also, it is not @escaping. The closure MUST run on the caller thread:

// On some thread
await withUnsafeContinuation { continuation in
    // On the same thread, 100% guaranteed
}

The lock is acquired before withUnsafeContinuation, and released inside withUnsafeContinuation because the Semaphore needs to prevent a concurrent call to signal or wait to mess with its inner state - made of both the value and the array of continuations to resume.

The semaphore value must be decremented before withUnsafeContinuation is called. The continuation is known inside withUnsafeContinuation. Hence the lock/unlock dance we could read.

Now, since withUnsafeContinuation spawns two "threads" of execution, one inside its closure, and one another by whatever task proceeds from the suspension point, could it be possible that the task creates a deadlock? Well, no. The task may have to wait for the lock to be released (a very short time), but this is not a deadlock. There is no cyclic dependency. unlock will run.

Is blocking a thread for the duration of withUnsafeContinuation a grave misuse of the Swift concurrency apis? Well, when the standard library provides the task synchronization apis we need (with quite possibly a few locks that we don't see), this library will stop to have any purpose, and the question will stop to have any relevance.

If withTaskCancellationHandler and withUnsafeThrowingContinuation also have behave like that it starts to make more sense for me.

This one introduces the need for a recursive lock. I have tried to explain why in the code - I hope the explanation is not too muddy.

[KaiOelfke]

Good point that the closure is not escaping.

To be honest I don't understand this:

"The task resumed from the suspension point of withUnsafeContinuation may want to proceed on the same thread before the closure is run."

Do you mean the suspension point here:

 public func wait() async {
        lock()
        // ...
        await withUnsafeContinuation { continuation in // <-- this await
            // ...
            unlock()
        }
    }

"The semantic rule has always been that the task is not resumed until both resume is called and the closure has returned."
Source: https://forums.swift.org/t/clarification-needed-on-unsafecontinuation-documentation/57803/9

It can resume on the same thread e.g. if it's all MainActor, with LIBDISPATCH_COOPERATIVE_POOL_STRICT, or on a less powerful device. But the task that called the initial wait() shouldn't be able to resume from await wait() or await withUnsafeContinuation before the continuation is resumed and the closure returned.

A different task running on the same or different thread can call something though.

Or are you talking about something else?

[groue]

"The task resumed from the suspension point of withUnsafeContinuation may want to proceed on the same thread before the closure is run."

withUnsafeContinuation is documented to suspend the current task, and then call the closure. When the current task is suspended, some other task may be resumed. The executor for this resumed task may schedule some code to run. For all I know, nothing prevents the executor to execute this code right away. And only then the closure is run.

Do you mean the suspension point here:

Yes.

"The semantic rule has always been that the task is not resumed until both resume is called and the closure has returned."
Source: https://forums.swift.org/t/clarification-needed-on-unsafecontinuation-documentation/57803/9

In this sentence, "the task" is the task that calls withUnsafeContinuation, not the eventual task that withUnsafeContinuation, as all suspension points, can resume.

But the task that calls withUnsafeContinuation is not the source of my questions. It is the other one, the one that is resumed from the withUnsafeContinuation suspension point.

And withUnsafeContinuation has big problems here. It can even ruin actor isolation (yes I should file a bug). See the sample code below:

Continuations can break actors 🤯

The following test instantiates one actor, and performs as many increments and decrements of an isolated count property. It tests that the final value is the initial one: zero.

func test_that_UnsafeContinuation_can_not_break_actor_isolation() async {
    actor MyActor {
        var count = 0
        let lock = NSLock()
        
        func zero() async {
            count += 1
            await withUnsafeContinuation { continuation in
                count -= 1
                DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) {
                    continuation.resume()
                }
            }
        }
        
        func zeroWithLock() async {
            lock.lock()
            count += 1
            await withUnsafeContinuation { continuation in
                count -= 1
                lock.unlock()
                DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) {
                    continuation.resume()
                }
            }
        }
    }
    
    do {
        let actor = MyActor()
        await withTaskGroup(of: Void.self) { group in
            for _ in 0..<1_000 {
                group.addTask {
                    await actor.zero() // 😖
                }
            }
        }
        
        let count = await actor.count
        // XCTAssertEqual failed: ("1") is not equal to ("0")
        // XCTAssertEqual failed: ("4") is not equal to ("0")
        // XCTAssertEqual failed: ("7") is not equal to ("0")
        XCTAssertEqual(count, 0)
    }
    
    do {
        let actor = MyActor()
        await withTaskGroup(of: Void.self) { group in
            for _ in 0..<1_000 {
                group.addTask {
                    await actor.zeroWithLock() // with a lock 😜
                }
            }
        }
        
        let count = await actor.count
        // Always good
        XCTAssertEqual(count, 0)
    }
}

[groue]

And withUnsafeContinuation has big problems here. It can even ruin actor isolation (yes I should file a bug).

Filed: apple/swift#61485

[rjmccall]

withUnsafeContinuation should not introduce a suspension point when called; the body should be called in the same thread without any interruption. So acquiring a lock prior to withUnsafeContinuation and releasing it within the callback should be fine.

This guarantee is unfortunately broken with Xcode 14 when compiling for macOS because it's shipping with an old macOS SDK that doesn't declare that withUnsafeContinuation inherits its caller's execution context. And yes, there is a related actor-isolation issue because of this bug. That will be fixed by the release of the new macOS SDK.

[groue]

Thank you @rjmccall for lifting the remaining doubts. I think we have a reliable semaphore, @KaiOelfke. Please ask other questions if you have other doubts.