Deadlock in inline snapshot testing

[groue]

Hello,

Happy new year :-)

I'm not sure if this is a bug report or a request for advice. But I could notice that inline snapshot testing can deadlock.

The first call to assertInlineSnapshot initializes a InlineSnapshotObserver: https://github.com/pointfreeco/swift-snapshot-testing/blob/1.15.1/Sources/InlineSnapshotTesting/AssertInlineSnapshot.swift#L270-L279

This initialization is protected by DispatchQueue.mainSync that checks for the current dispatch queue: https://github.com/pointfreeco/swift-snapshot-testing/blob/1.15.1/Sources/InlineSnapshotTesting/AssertInlineSnapshot.swift#L285-L292

If, unfortunately, the first call to assertInlineSnapshot is made, synchronously, from a dispatch queue which is not the main queue, we have a deadlock:

func test_something() {
    let queue = DispatchQueue(...)
    queue.sync {
        // deadlock
        assertInlineSnapshot(...)
    }
}

Maybe snapshot testing MUST be done from the main thread, and the above test is illegal?

Or maybe not? Indeed, dispatch queues are a valid way to protect access to shared mutable resources, and testing such resources is easier when the test can be executed right from such dispatch queue.

What would you suggest?

[stephencelis]

@groue The main thread requirement is from XCTest's observer registration. XCTest has a bunch of hidden, main thread requirements that, when done from a non-main thread, cause an XCTest run to raise an exception.

We could make the API for installing the test observer public in cases where someone needs to execute their first snapshot test on a non-main thread, but ideally things would remain transparent.

Do you have any alternate ideas for ensuring this observer registration code runs on the main thread?

[groue]

Hi @stephencelis, thanks for the explanation :-)

Maybe replacing the getSpecific check with Thread.isMainThread would still comply with XCTest reliance on the main thread... But it's not an easy question: some code actually relies on the main queue very precisely. Maybe XCTest will eventually rely on the main queue, or some main actor executor, what do I know 😅 Maybe the implicit initialization of InlineSnapshotObserver is convenient, but fragile? Also, I lack experience regarding Linux support.

We could make the API for installing the test observer public in cases where someone needs to execute their first snapshot test on a non-main thread, but ideally things would remain transparent.

I also contemplate this ideal :-) Considering my first paragraph, and our my uncertainty about the inner working of XCTest, a public api that clients would have to explicitly call from the main queue (from the setUp override, probably) would be a safe bet. I mean, I wouldn't complain.

[stephencelis]

Maybe replacing the getSpecific check with Thread.isMainThread would still comply with XCTest reliance on the main thread...

We still need to register it synchronously on the main thread, though, so wouldn't this have the same problem if you're on a non-main thread but being synchronously dispatched to from the main thread? Or maybe I'm forgetting some of the specifics of dispatch here.

But it's not an easy question: some code actually relies on the main queue very precisely. Maybe XCTest will eventually rely on the main queue, or some main actor executor, what do I know 😅 Maybe the implicit initialization of InlineSnapshotObserver is convenient, but fragile?

XCTest might! But I'd be surprised if it sees much iteration with swift-testing being worked on.

Also, I lack experience regarding Linux support.

Linux supports test observers just fine, so should not have much to worry about here 😄

[groue]

We still need to register it synchronously on the main thread, though, so wouldn't this have the same problem if you're on a non-main thread but being synchronously dispatched to from the main thread? Or maybe I'm forgetting some of the specifics of dispatch here.

I was considering something like this:

private let installTestObserver: Void = {
  final class InlineSnapshotObserver: NSObject, XCTestObservation {
    func testBundleDidFinish(_ testBundle: Bundle) {
      writeInlineSnapshots()
    }
  }
  // XCTest has a bunch of hidden, main thread requirements that, when done
  // from a non-main thread, cause an XCTest run to raise an exception. This
  // is one of them.
  let observer = InlineSnapshotObserver()
  if Thread.isMainThread {
    XCTestObservationCenter.shared.addTestObserver(observer)
  } else {
    // Might dead-lock. Still looking for a nice workaround.
    DispatchQueue.main.sync {
      XCTestObservationCenter.shared.addTestObserver(observer)
    }
  }
}()

This would work in the sample code below, because DispatchQueue.sync uses the current thread whenever it can:

[...] As a performance optimization, this function executes blocks on the current thread whenever possible, with one exception: Blocks submitted to the main dispatch queue always run on the main thread.

func test_something() {
    // On main thread
    let queue = DispatchQueue(...)
    queue.sync {
        // In `queue`, but still on main thread: no deadlock
        assertInlineSnapshot(...)
    }
}

XCTest might! But I'd be surprised if it sees much iteration with swift-testing being worked on.

Fair point :-)