Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Make models Sendable #829

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: Make models Sendable #829

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
60a44e7
feat: Mark Document and ByteStream as Sendable
jbelkins Sep 20, 2024
9561ade
Make models Sendable
jbelkins Sep 21, 2024
65ce5bc
Fix tests
jbelkins Sep 21, 2024
95767ce
Deleted extensions on Swift & CRT types
jbelkins Sep 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions Sources/ClientRuntime/PrimitiveTypeExtensions/Indirect.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,28 @@
// SPDX-License-Identifier: Apache-2.0
//

import class Foundation.NSRecursiveLock

@propertyWrapper
public class Indirect<T> {
public var wrappedValue: Optional<T>
public final class Indirect<T: Sendable>: @unchecked Sendable {
private let lock = NSRecursiveLock()
private var _wrappedValue: Optional<T>

public var wrappedValue: Optional<T> {
get {
lock.lock()
defer { lock.unlock() }
return _wrappedValue
}
set {
lock.lock()
defer { lock.unlock() }
_wrappedValue = newValue
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property wrapper is mostly same as before, but now stores its value in private storage that is protected by a lock when getting & setting.

Since it enforces exclusive access to its wrapped value, it can now be Sendable.


public init(wrappedValue: Optional<T>) {
self.wrappedValue = wrappedValue
self._wrappedValue = wrappedValue
}
}

Expand Down
2 changes: 1 addition & 1 deletion Sources/Smithy/ByteStream.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import class Foundation.FileHandle
import struct Foundation.Data

public enum ByteStream {
public enum ByteStream: Sendable {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conformance is possible since Stream has been made Sendable in this PR.

case data(Data?)
case stream(Stream)
case noStream
Expand Down
17 changes: 16 additions & 1 deletion Sources/Smithy/Document/SmithyDocument.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@
import struct Foundation.Data
import struct Foundation.Date

public protocol SmithyDocument {
public protocol SmithyDocument: Sendable {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the SmithyDocument implementations are now sendable without any code change, since they only store data that is either immutable and/or Sendable.


/// The Smithy type corresponding to the data stored in this document.
var type: ShapeType { get }

// "as" methods throw if the document doesn't match the requested type.
Expand Down Expand Up @@ -116,6 +117,20 @@ public extension SmithyDocument {

extension SmithyDocument {

/// Compares two `SmithyDocument`-conforming values, checking for equality.
///
/// Two `SmithyDocument`s are equal if they have the same type and equal values.
///
/// Two Smithy `list` documents are equal if they have equal documents at every index.
///
/// Two Smithy `map` documents are equal if they have the same set of keys, and equal values for every key.
///
/// - note: Because `SmithyDocument` is a protocol, it cannot conform to `Equatable`; the type-erased
/// container type ``Document`` is used to provide Smithy documents with equatability.
/// - Parameters:
/// - lhs: The first `SmithyDocument` to compare.
/// - rhs: The second `SmithyDocument` to compare.
/// - Returns: `true` if the two `SmithyDocument`s are equal, `false` otherwise.
public static func isEqual(_ lhs: SmithyDocument, _ rhs: SmithyDocument) -> Bool {
switch (lhs.type, rhs.type) {
case (.blob, .blob):
Expand Down
4 changes: 2 additions & 2 deletions Sources/Smithy/Stream.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import struct Foundation.Data

/// Protocol that provides reading data from a stream
public protocol ReadableStream: AnyObject {
public protocol ReadableStream: AnyObject, Sendable {
/// Returns the current position in the stream
var position: Data.Index { get }

Expand Down Expand Up @@ -45,7 +45,7 @@ public protocol ReadableStream: AnyObject {
}

/// Protocol that provides writing data to a stream
public protocol WriteableStream: AnyObject {
public protocol WriteableStream: AnyObject, Sendable {
/// Writes the contents of `data` to the stream
/// - Parameter data: data to write
func write(contentsOf data: Data) throws
Expand Down
4 changes: 2 additions & 2 deletions Sources/SmithyChecksums/ChunkedStream.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ import class SmithyStreams.BufferedStream
/// URLSessionHTTPClient streams chunked payloads using this stream type.
/// CRTClientEngine uses only the reader provided by this type to create chunks, then it
/// streams them itself.
public class ChunkedStream {
private var inputStream: Stream
public class ChunkedStream: @unchecked Sendable {
private let inputStream: Stream
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Streams were designed to be thread-safe so they may be marked @unchecked Sendable so long as we maintain that thread safety.

private var signingConfig: SigningConfig
private var previousSignature: String
private var trailingHeaders: Headers
Expand Down
2 changes: 1 addition & 1 deletion Sources/SmithyChecksums/ValidatingBufferedStream.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import struct Foundation.Data
import AwsCommonRuntimeKit
import class SmithyStreams.BufferedStream

public class ValidatingBufferedStream {
public class ValidatingBufferedStream: @unchecked Sendable {
private var stream: BufferedStream
private var checksumAlgorithm: ChecksumAlgorithm
private var checksum: (any Checksum)
Expand Down
2 changes: 1 addition & 1 deletion Sources/SmithyEventStreams/DefaultMessageEncoderStream.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import SmithyEventStreamsAuthAPI
import struct Foundation.Data

/// Stream adapter that encodes input into `Data` objects.
public class DefaultMessageEncoderStream<Event>: MessageEncoderStream, Stream {
public class DefaultMessageEncoderStream<Event>: MessageEncoderStream, Stream, @unchecked Sendable {

let stream: AsyncThrowingStream<Event, Error>
let messageEncoder: MessageEncoder
Expand Down
14 changes: 7 additions & 7 deletions Sources/SmithyEventStreamsAPI/EventStreamError.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@
case invalidMessage(String)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extension below was unused and causing compiler warnings, so it was deleted.

extension AsyncThrowingStream: Equatable where Element: Equatable {

public static func == (lhs: AsyncThrowingStream, rhs: AsyncThrowingStream) -> Bool {
// TODO: Remove as part of https://github.com/awslabs/aws-sdk-swift/issues/898
return false
}
}
//extension AsyncThrowingStream: Equatable where Element: Equatable {

Check warning on line 19 in Sources/SmithyEventStreamsAPI/EventStreamError.swift

View workflow job for this annotation

GitHub Actions / swiftlint 

Prefer at least one space after slashes for comments (comment_spacing)
//
// public static func == (lhs: AsyncThrowingStream, rhs: AsyncThrowingStream) -> Bool {
// // TODO: Remove as part of https://github.com/awslabs/aws-sdk-swift/issues/898
// return false
// }
//}

Check warning on line 25 in Sources/SmithyEventStreamsAPI/EventStreamError.swift

View workflow job for this annotation

GitHub Actions / swiftlint 

Prefer at least one space after slashes for comments (comment_spacing)
4 changes: 2 additions & 2 deletions Sources/SmithyHTTPAPI/Headers.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
// SPDX-License-Identifier: Apache-2.0
//

public struct Headers {
public struct Headers: Sendable {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Headers and Header may be marked Sendable without any further change since they are structs that only store value types.

public var headers: [Header] = []

/// Creates an empty instance.
Expand Down Expand Up @@ -197,7 +197,7 @@ extension Array where Element == Header {
}
}

public struct Header {
public struct Header: Sendable {
public var name: String
public var value: [String]

Expand Down
2 changes: 1 addition & 1 deletion Sources/SmithyStreams/BufferedStream.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import protocol Smithy.Stream
/// Note: This class is thread-safe and async-safe.
/// Note: if data is not read from the stream, the buffer will grow indefinitely until the stream is closed.
/// or reach the maximum size of a `Data` object.
public class BufferedStream: Stream {
public class BufferedStream: Stream, @unchecked Sendable {

/// Returns the cumulative length of all data so far written to the stream, if known.
/// For a buffered stream, the length will only be known if the stream has closed.
Expand Down
4 changes: 2 additions & 2 deletions Sources/SmithyStreams/FileStream.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import protocol Smithy.Stream
/// A `Stream` that wraps a `FileHandle` for reading the file.
///
/// - Note: This class is thread-safe.
public final class FileStream: Stream {
public final class FileStream: Stream, @unchecked Sendable {

/// Returns the length of the stream, if known
public var length: Int? {
Expand All @@ -26,7 +26,7 @@ public final class FileStream: Stream {
let fileHandle: FileHandle

/// Returns the current position of the stream.
public var position: Data.Index
public private(set) var position: Data.Index
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this is a breaking API change but it would cause a bug to alter this property from outside this type.


/// Returns true if length is zero, false otherwise.
public var isEmpty: Bool {
Expand Down
3 changes: 2 additions & 1 deletion smithy-swift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/EnumGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,9 @@ class EnumGenerator(
writer.writeShapeDocs(shape)
writer.writeAvailableAttribute(null, shape)
writer.openBlock(
"public enum \$enum.name:L: \$N, \$N, \$N, \$N {",
"public enum \$enum.name:L: \$N, \$N, \$N, \$N, \$N {",
"}",
SwiftTypes.Protocols.Sendable,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark generated enums with Sendable.

SwiftTypes.Protocols.Equatable,
SwiftTypes.Protocols.RawRepresentable,
SwiftTypes.Protocols.CaseIterable,
Expand Down
4 changes: 3 additions & 1 deletion ...hy-swift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/IntEnumGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class IntEnumGenerator(
if (isNestedType) {
val service = model.expectShape<ServiceShape>(settings.service)
writer.openBlock("extension ${service.nestedNamespaceType(symbolProvider)} {", "}") {
writer.write("")
renderEnum()
}
} else {
Expand All @@ -41,8 +42,9 @@ class IntEnumGenerator(
writer.writeShapeDocs(shape)
writer.writeAvailableAttribute(null, shape)
writer.openBlock(
"public enum \$enum.name:L: \$N, \$N, \$N, \$N {",
"public enum \$enum.name:L: \$N, \$N, \$N, \$N, \$N {",
"}",
SwiftTypes.Protocols.Sendable,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark generated intEnums with Sendable.

Also adjust spacing a bit.

SwiftTypes.Protocols.Equatable,
SwiftTypes.Protocols.RawRepresentable,
SwiftTypes.Protocols.CaseIterable,
Expand Down
6 changes: 3 additions & 3 deletions ...-swift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/StructureGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,15 +113,15 @@ class StructureGenerator(
}

private fun generateStruct() {
writer.write("")
writer.writeShapeDocs(shape)
writer.writeAvailableAttribute(model, shape)
val equatableConformance = writer.format(": \$N ", SwiftTypes.Protocols.Equatable).takeIf { shape.hasTrait<EquatableConformanceTrait>() } ?: ""
writer.openBlock("public struct \$struct.name:L $equatableConformance{")
val equatableConformance = writer.format(", \$N", SwiftTypes.Protocols.Equatable).takeIf { shape.hasTrait<EquatableConformanceTrait>() } ?: ""
writer.openBlock("public struct \$struct.name:L: \$N$equatableConformance {", SwiftTypes.Protocols.Sendable)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark generated structs with Sendable.

Also adjust spacing of generated code.

.call { generateStructMembers() }
.write("")
.call { generateInitializerForStructure(false) }
.closeBlock("}")
.write("")
}

private fun generateStructMembers() {
Expand Down
5 changes: 3 additions & 2 deletions smithy-swift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/UnionGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ class UnionGenerator(
if (isNestedType) {
val service = model.expectShape<ServiceShape>(settings.service)
writer.openBlock("extension ${service.nestedNamespaceType(symbolProvider)} {", "}") {
writer.write("")
renderUnion()
}
} else {
Expand All @@ -76,8 +77,8 @@ class UnionGenerator(
writer.writeShapeDocs(shape)
writer.writeAvailableAttribute(model, shape)
val indirectOrNot = "indirect ".takeIf { shape.hasTrait<RecursiveUnionTrait>() } ?: ""
val equatableConformance = (": " + SwiftTypes.Protocols.Equatable + " ").takeIf { shape.hasTrait<EquatableConformanceTrait>() } ?: ""
writer.openBlock("public ${indirectOrNot}enum \$union.name:L $equatableConformance{", "}\n") {
val equatableConformance = writer.format(", \$N", SwiftTypes.Protocols.Equatable).takeIf { shape.hasTrait<EquatableConformanceTrait>() } ?: ""
writer.openBlock("public ${indirectOrNot}enum \$union.name:L: \$N$equatableConformance {", "}", SwiftTypes.Protocols.Sendable) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark generated unions with Sendable.

Also adjust formatting.

// event streams (@streaming union) MAY have variants that target errors.
// These errors if encountered on the stream will be thrown as an exception rather
// than showing up as one of the possible events the consumer will see on the stream (AsyncThrowingStream<T>).
Expand Down
1 change: 1 addition & 0 deletions ...t-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/swiftmodules/SwiftTypes.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ object SwiftTypes {
val RawRepresentable = builtInSymbol("RawRepresentable", SwiftDeclaration.PROTOCOL)
val CaseIterable = builtInSymbol("CaseIterable", SwiftDeclaration.PROTOCOL)
val CustomDebugStringConvertible = builtInSymbol("CustomDebugStringConvertible", SwiftDeclaration.PROTOCOL)
val Sendable = builtInSymbol("Sendable", SwiftDeclaration.PROTOCOL)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SwiftSymbol for the Sendable protocol.

}
}

Expand Down
6 changes: 3 additions & 3 deletions smithy-swift-codegen/src/test/kotlin/EnumGeneratorTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class EnumGeneratorTests {

val expectedGeneratedEnum = """
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below changes are Kotlin codegen tests. Changes in test expectations:

  • All generated structs, unions, enums, & intEnums are Sendable.
  • Whitespace around type declarations is made uniform, with an empty line before a declaration & none after.
  • Test strings are left-justified to allow for easy maintenance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try turning off whitespace changes on the Github diff to see the more substantial part of the test changes.

/// Really long multi-line Documentation for the enum
public enum MyEnum: Swift.Equatable, Swift.RawRepresentable, Swift.CaseIterable, Swift.Hashable {
public enum MyEnum: Swift.Sendable, Swift.Equatable, Swift.RawRepresentable, Swift.CaseIterable, Swift.Hashable {
/// Documentation for BAR
case bar
case fooBazXap
Expand Down Expand Up @@ -98,7 +98,7 @@ public enum MyEnum: Swift.Equatable, Swift.RawRepresentable, Swift.CaseIterable,

val expectedGeneratedEnum = """
/// Really long multi-line Documentation for the enum
public enum MyEnum: Swift.Equatable, Swift.RawRepresentable, Swift.CaseIterable, Swift.Hashable {
public enum MyEnum: Swift.Sendable, Swift.Equatable, Swift.RawRepresentable, Swift.CaseIterable, Swift.Hashable {
/// ""${'"'} T2 instances are Burstable Performance Instances that provide a baseline level of CPU performance with the ability to burst above the baseline.""${'"'}
case t2Micro
case t2Nano
Expand Down Expand Up @@ -141,7 +141,7 @@ public enum MyEnum: Swift.Equatable, Swift.RawRepresentable, Swift.CaseIterable,
var expectedGeneratedEnum = """
extension ExampleClientTypes {

public enum Suit: Swift.Equatable, Swift.RawRepresentable, Swift.CaseIterable, Swift.Hashable {
public enum Suit: Swift.Sendable, Swift.Equatable, Swift.RawRepresentable, Swift.CaseIterable, Swift.Hashable {
case club
case diamond
case heart
Expand Down
2 changes: 1 addition & 1 deletion smithy-swift-codegen/src/test/kotlin/IntEnumGeneratorTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class IntEnumGeneratorTests {
.getFileString("Sources/example/models/Abcs.swift").get()
Assertions.assertNotNull(enumShape)
var expectedGeneratedEnum = """
public enum Abcs: Swift.Equatable, Swift.RawRepresentable, Swift.CaseIterable, Swift.Hashable {
public enum Abcs: Swift.Sendable, Swift.Equatable, Swift.RawRepresentable, Swift.CaseIterable, Swift.Hashable {
case a
case b
case c
Expand Down
8 changes: 4 additions & 4 deletions smithy-swift-codegen/src/test/kotlin/PaginatorGeneratorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ class PaginatorGeneratorTest {
// Equatable conformance must have been generated for struct nested inside a pagination token.
val contents = getFileContents(context.manifest, "Sources/Test/models/NestedInputTokenValue.swift")
val expected = """
public struct NestedInputTokenValue : Swift.Equatable {
public struct NestedInputTokenValue: Swift.Sendable, Swift.Equatable {
""".trimIndent()
contents.shouldContainOnlyOnce(expected)
}
Expand All @@ -158,7 +158,7 @@ class PaginatorGeneratorTest {
// Equatable conformance must have been generated for struct nested under pagination token.
val contents = getFileContents(context.manifest, "Sources/Test/models/DoublyNestedInputTokenValue.swift")
val expected = """
public struct DoublyNestedInputTokenValue : Swift.Equatable {
public struct DoublyNestedInputTokenValue: Swift.Sendable, Swift.Equatable {
""".trimIndent()
contents.shouldContainOnlyOnce(expected)
}
Expand All @@ -169,8 +169,8 @@ class PaginatorGeneratorTest {
// Equatable conformance must have been generated for union nested under pagination token.
val contents = getFileContents(context.manifest, "Sources/Test/models/InputPaginationUnion.swift")
val expected = """
public enum InputPaginationUnion : Swift.Equatable {
""".trimIndent()
public enum InputPaginationUnion: Swift.Sendable, Swift.Equatable {
"""
contents.shouldContainOnlyOnce(expected)
}

Expand Down
Loading
Loading