From 58d6f8ccf6780b77aad620791de24c25a7f2983c Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Tue, 8 Apr 2025 18:09:41 -0400 Subject: [PATCH 1/2] Don't use a class to store the current exit test. This PR replaces the private `_CurrentContainer` class used to store the current exit test with a bare pointer. The class is prone to duplicate definitions, but we really just use it as a glorified box type for the move-only `ExitTest`, so an immortal pointer will work just as well. Works around rdar://148837303. --- Sources/Testing/ExitTests/ExitTest.swift | 33 +++++++----------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/Sources/Testing/ExitTests/ExitTest.swift b/Sources/Testing/ExitTests/ExitTest.swift index 6341ce422..3254a94dd 100644 --- a/Sources/Testing/ExitTests/ExitTest.swift +++ b/Sources/Testing/ExitTests/ExitTest.swift @@ -114,24 +114,15 @@ public struct ExitTest: Sendable, ~Copyable { @_spi(Experimental) extension ExitTest { - /// A container type to hold the current exit test. - /// - /// This class is temporarily necessary until `ManagedBuffer` is updated to - /// support storing move-only values. For more information, see [SE-NNNN](https://github.com/swiftlang/swift-evolution/pull/2657). - private final class _CurrentContainer: Sendable { - /// The exit test represented by this container. - /// - /// The value of this property must be optional to avoid a copy when reading - /// the value in ``ExitTest/current``. - let exitTest: ExitTest? - - init(exitTest: borrowing ExitTest) { - self.exitTest = ExitTest(id: exitTest.id, body: exitTest.body, _observedValues: exitTest._observedValues) - } - } - /// Storage for ``current``. - private static let _current = Locked<_CurrentContainer?>() + /// + /// A pointer is used for indirection because `ManagedBuffer` cannot yet hold + /// move-only types. + private static nonisolated(unsafe) let _current: Locked> = { + let result = UnsafeMutablePointer.allocate(capacity: 1) + result.initialize(to: nil) + return Locked(rawValue: result) + }() /// The exit test that is running in the current process, if any. /// @@ -144,11 +135,7 @@ extension ExitTest { /// process. public static var current: ExitTest? { _read { - if let current = _current.rawValue { - yield current.exitTest - } else { - yield nil - } + yield _current.rawValue.pointee } } } @@ -235,7 +222,7 @@ extension ExitTest { // Set ExitTest.current before the test body runs. Self._current.withLock { current in - current = _CurrentContainer(exitTest: self) + current.pointee = ExitTest(id: id, body: body, _observedValues: _observedValues) } do { From 72bb4039cebbf65e059a5203c2721098e09f585b Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Wed, 9 Apr 2025 11:27:55 -0400 Subject: [PATCH 2/2] Avoid thread-unsafe yielding of the current exit test --- Sources/Testing/ExitTests/ExitTest.swift | 31 +++++++++++++++++++----- Sources/Testing/Support/Locked.swift | 4 +-- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/Sources/Testing/ExitTests/ExitTest.swift b/Sources/Testing/ExitTests/ExitTest.swift index 3254a94dd..95c3ec696 100644 --- a/Sources/Testing/ExitTests/ExitTest.swift +++ b/Sources/Testing/ExitTests/ExitTest.swift @@ -107,6 +107,18 @@ public struct ExitTest: Sendable, ~Copyable { _observedValues = newValue } } + + /// Make a copy of this instance. + /// + /// - Returns: A copy of this instance. + /// + /// This function is unsafe because if the caller is not careful, it could + /// invoke the same exit test twice. + fileprivate borrowing func unsafeCopy() -> Self { + var result = Self(id: id, body: body) + result._observedValues = _observedValues + return result + } } #if !SWT_NO_EXIT_TESTS @@ -118,10 +130,10 @@ extension ExitTest { /// /// A pointer is used for indirection because `ManagedBuffer` cannot yet hold /// move-only types. - private static nonisolated(unsafe) let _current: Locked> = { - let result = UnsafeMutablePointer.allocate(capacity: 1) - result.initialize(to: nil) - return Locked(rawValue: result) + private static nonisolated(unsafe) var _current: Locked> = { + let current = UnsafeMutablePointer.allocate(capacity: 1) + current.initialize(to: nil) + return Locked(rawValue: current) }() /// The exit test that is running in the current process, if any. @@ -135,7 +147,13 @@ extension ExitTest { /// process. public static var current: ExitTest? { _read { - yield _current.rawValue.pointee + // NOTE: Even though this accessor is `_read` and has borrowing semantics, + // we must make a copy so that we don't yield lock-guarded memory to the + // caller (which is not concurrency-safe.) + let currentCopy = _current.withLock { current in + return current.pointee?.unsafeCopy() + } + yield currentCopy } } } @@ -222,7 +240,8 @@ extension ExitTest { // Set ExitTest.current before the test body runs. Self._current.withLock { current in - current.pointee = ExitTest(id: id, body: body, _observedValues: _observedValues) + precondition(current.pointee == nil, "Set the current exit test twice in the same process. Please file a bug report at https://github.com/swiftlang/swift-testing/issues/new") + current.pointee = self.unsafeCopy() } do { diff --git a/Sources/Testing/Support/Locked.swift b/Sources/Testing/Support/Locked.swift index e8b17be7b..d1db8ef1f 100644 --- a/Sources/Testing/Support/Locked.swift +++ b/Sources/Testing/Support/Locked.swift @@ -88,7 +88,7 @@ struct LockedWith: RawRepresentable where L: Lockable { /// This function can be used to synchronize access to shared data from a /// synchronous caller. Wherever possible, use actor isolation or other Swift /// concurrency tools. - nonmutating func withLock(_ body: (inout T) throws -> R) rethrows -> R { + nonmutating func withLock(_ body: (inout T) throws -> R) rethrows -> R where R: ~Copyable { try _storage.withUnsafeMutablePointers { rawValue, lock in L.unsafelyAcquireLock(at: lock) defer { @@ -118,7 +118,7 @@ struct LockedWith: RawRepresentable where L: Lockable { /// - Warning: Callers that unlock the lock _must_ lock it again before the /// closure returns. If the lock is not acquired when `body` returns, the /// effect is undefined. - nonmutating func withUnsafeUnderlyingLock(_ body: (UnsafeMutablePointer, T) throws -> R) rethrows -> R { + nonmutating func withUnsafeUnderlyingLock(_ body: (UnsafeMutablePointer, T) throws -> R) rethrows -> R where R: ~Copyable { try withLock { value in try _storage.withUnsafeMutablePointerToElements { lock in try body(lock, value)