From f245ae63b3f3963cc4f15765fdfa2f108b8adaf0 Mon Sep 17 00:00:00 2001 From: Dmitry Kovba Date: Wed, 11 Jun 2025 11:13:21 -0700 Subject: [PATCH] Improve `@SendableProperty` (#91) This PR ensures that we enter a lock inside the `@SendableProperty` implementation as soon as we access a computed property. Additionally, it mirrors the access level of the original property. Both changes are required for [improved accuracy](https://github.com/apple/container/pull/144) of progress updates in container. Additionally, it should resolve https://github.com/apple/containerization/issues/60 that occurs on certain configurations. Please tag as 0.1.1 after merging. --- Sources/Containerization/LinuxContainer.swift | 1 + .../Client/RegistryClient+Fetch.swift | 6 ++-- .../SendableProperty/SendableProperty.swift | 26 +++++++++++++-- .../SendablePropertyMacro.swift | 16 +++------ .../ImageTests/ImageStoreTests.swift | 2 +- .../SendablePropertyMacrosTests.swift | 33 ++++--------------- 6 files changed, 39 insertions(+), 45 deletions(-) diff --git a/Sources/Containerization/LinuxContainer.swift b/Sources/Containerization/LinuxContainer.swift index 8613f19c..2f2a8ad1 100644 --- a/Sources/Containerization/LinuxContainer.swift +++ b/Sources/Containerization/LinuxContainer.swift @@ -22,6 +22,7 @@ import ContainerizationOCI import Foundation import Logging import SendableProperty +import Synchronization import struct ContainerizationOS.Terminal diff --git a/Sources/ContainerizationOCI/Client/RegistryClient+Fetch.swift b/Sources/ContainerizationOCI/Client/RegistryClient+Fetch.swift index 4ee41ea2..043a9b18 100644 --- a/Sources/ContainerizationOCI/Client/RegistryClient+Fetch.swift +++ b/Sources/ContainerizationOCI/Client/RegistryClient+Fetch.swift @@ -177,10 +177,10 @@ extension RegistryClient { while var buf = try await itr.next() { let readBytes = Int64(buf.readableBytes) received += readBytes - await progress?([ - ProgressEvent(event: "add-size", value: readBytes) - ]) let written = try await writer.write(contentsOf: buf) + await progress?([ + ProgressEvent(event: "add-size", value: written) + ]) guard written == readBytes else { throw ContainerizationError(.internalError, message: "Could not write \(readBytes) bytes to file \(file)") } diff --git a/Sources/SendableProperty/SendableProperty.swift b/Sources/SendableProperty/SendableProperty.swift index 33cea110..345a541b 100644 --- a/Sources/SendableProperty/SendableProperty.swift +++ b/Sources/SendableProperty/SendableProperty.swift @@ -15,10 +15,32 @@ // limitations under the License. //===----------------------------------------------------------------------===// -// `Synchronization` will be automatically imported with `SendableProperty` -@_exported import Synchronization +// `Foundation` will be automatically imported with `SendableProperty`. +@_exported import Foundation // A declaration of the `@SendableProperty` macro. @attached(peer, names: arbitrary) @attached(accessor) public macro SendableProperty() = #externalMacro(module: "SendablePropertyMacros", type: "SendablePropertyMacro") + +/// A synchronization primitive that protects shared mutable state via mutual exclusion. +public final class Synchronized: @unchecked Sendable { + private let lock = NSLock() + private var value: T + + /// Creates a new instance. + /// - Parameter value: The initial value. + public init(_ value: T) { + self.value = value + } + + /// Calls the given closure after acquiring the lock and returns its value. + /// - Parameter body: The body of code to execute while the lock is held. + public func withLock(_ body: (inout T) throws -> R) rethrows -> R { + lock.lock() + defer { + lock.unlock() + } + return try body(&value) + } +} diff --git a/Sources/SendablePropertyMacros/SendablePropertyMacro.swift b/Sources/SendablePropertyMacros/SendablePropertyMacro.swift index 7764daeb..14644102 100644 --- a/Sources/SendablePropertyMacros/SendablePropertyMacro.swift +++ b/Sources/SendablePropertyMacros/SendablePropertyMacro.swift @@ -21,7 +21,6 @@ import SwiftParser import SwiftSyntax import SwiftSyntaxBuilder import SwiftSyntaxMacros -import Synchronization /// A macro that allows to make a property thread-safe keeping the `Sendable` conformance of the type. public struct SendablePropertyMacro: PeerMacro { @@ -54,12 +53,13 @@ public struct SendablePropertyMacro: PeerMacro { genericTypeAnnotation = "<\(typeName)\(hasInitializer ? "" : "?")>" } + let accessLevel = varDecl.modifiers.first(where: { ["open", "public", "internal", "fileprivate", "private"].contains($0.name.text) })?.name.text ?? "internal" + // Create a peer property let peerPropertyName = self.peerPropertyName(for: propertyName) - // `Mutex` (requires macOS 15) and `OSAllocationUnfairLock` (requires macOS 13, unsupported on Linux) are more effective than `NSLock`. let peerProperty: DeclSyntax = """ - private let \(raw: peerPropertyName) = Mutex\(raw: genericTypeAnnotation)(\(raw: initializerValue)) + \(raw: accessLevel) let \(raw: peerPropertyName) = Synchronized\(raw: genericTypeAnnotation)(\(raw: initializerValue)) """ return [peerProperty] } @@ -93,18 +93,10 @@ extension SendablePropertyMacro: AccessorMacro { \(raw: peerPropertyName).withLock { $0\(raw: hasInitializer ? "" : "!") } } """ - // The `Sending` class is used as a temporary workaround for the error: "'inout sending' parameter '$0' cannot be task-isolated at end of function." let accessorSetter: AccessorDeclSyntax = """ set { - class Sending: @unchecked Sendable { - let wrappedValue: T - init(_ value: T) { - wrappedValue = value - } - } - let newValue = Sending(newValue) - \(raw: peerPropertyName).withLock { $0 = newValue.wrappedValue } + \(raw: peerPropertyName).withLock { $0 = newValue } } """ diff --git a/Tests/ContainerizationTests/ImageTests/ImageStoreTests.swift b/Tests/ContainerizationTests/ImageTests/ImageStoreTests.swift index b4ecf775..6e7f1f6c 100644 --- a/Tests/ContainerizationTests/ImageTests/ImageStoreTests.swift +++ b/Tests/ContainerizationTests/ImageTests/ImageStoreTests.swift @@ -62,7 +62,7 @@ public class ImageStoreTests: ContainsAuth { return } let imageReference = "ghcr.io/apple/containerization/dockermanifestimage:0.0.2" - let busyboxImage = try await self.store.pull(reference: imageReference, auth: Self.authentication) + let busyboxImage = try await self.store.pull(reference: imageReference, auth: authentication) let got = try await self.store.get(reference: imageReference) #expect(got.descriptor == busyboxImage.descriptor) diff --git a/Tests/SendablePropertyMacrosTests/SendablePropertyMacrosTests.swift b/Tests/SendablePropertyMacrosTests/SendablePropertyMacrosTests.swift index 696f9476..52e69f62 100644 --- a/Tests/SendablePropertyMacrosTests/SendablePropertyMacrosTests.swift +++ b/Tests/SendablePropertyMacrosTests/SendablePropertyMacrosTests.swift @@ -52,20 +52,13 @@ final class SendablePropertyMacrosTests: XCTestCase { } } set { - class Sending: @unchecked Sendable { - let wrappedValue: T - init(_ value: T) { - wrappedValue = value - } - } - let newValue = Sending(newValue) _value.withLock { - $0 = newValue.wrappedValue + $0 = newValue } } } - private let _value = Mutex(nil) + internal let _value = Synchronized(nil) } """, macros: testMacros @@ -94,20 +87,13 @@ final class SendablePropertyMacrosTests: XCTestCase { } } set { - class Sending: @unchecked Sendable { - let wrappedValue: T - init(_ value: T) { - wrappedValue = value - } - } - let newValue = Sending(newValue) _value.withLock { - $0 = newValue.wrappedValue + $0 = newValue } } } - private let _value = Mutex(0) + internal let _value = Synchronized(0) } """, macros: testMacros @@ -136,20 +122,13 @@ final class SendablePropertyMacrosTests: XCTestCase { } } set { - class Sending: @unchecked Sendable { - let wrappedValue: T - init(_ value: T) { - wrappedValue = value - } - } - let newValue = Sending(newValue) _value.withLock { - $0 = newValue.wrappedValue + $0 = newValue } } } - private let _value = Mutex(0) + internal let _value = Synchronized(0) } """, macros: testMacros