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.

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

chore: Make modeled errors Sendable #883

Merged
merged 5 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
47 changes: 26 additions & 21 deletions Sources/SmithyHTTPAPI/HTTPResponse.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,43 +8,48 @@
import protocol Smithy.ResponseMessage
import protocol Smithy.Stream
import enum Smithy.ByteStream
import class Foundation.DispatchQueue
import class Foundation.NSRecursiveLock

public class HTTPResponse: HTTPURLResponse, ResponseMessage {
public final class HTTPResponse: ResponseMessage, @unchecked Sendable {
private var lock = NSRecursiveLock()
Copy link
Contributor Author

@jbelkins jbelkins Dec 10, 2024

Choose a reason for hiding this comment

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

The headers, body, and statusCode properties on HTTPResponse are locked with a recursive lock.

(A DispatchQueue was previously used on statusCode; it has been converted to NSRecursiveLock for simplicity, and also because use of GCD is discouraged in Swift concurrency code.)


public var headers: Headers
public var body: ByteStream
public var reason: String?
private var _headers: Headers
public var headers: Headers {
get { lock.lock(); defer { lock.unlock() }; return _headers }
set { lock.lock(); defer { lock.unlock() }; self._headers = newValue }
}

private var _body: ByteStream
public var body: ByteStream {
get { lock.lock(); defer { lock.unlock() }; return _body }
set { lock.lock(); defer { lock.unlock() }; self._body = newValue }
}

private var _statusCode: HTTPStatusCode
private let statusCodeQueue = DispatchQueue(label: "statusCodeSerialQueue")
public var statusCode: HTTPStatusCode {
get {
statusCodeQueue.sync {
return _statusCode
}
}
set {
statusCodeQueue.sync {
self._statusCode = newValue
}
}
get { lock.lock(); defer { lock.unlock() }; return _statusCode }
set { lock.lock(); defer { lock.unlock() }; self._statusCode = newValue }
}

public let reason: 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 reason property is never used, I do not know what the intended use is for it.
It has been converted to read-only.


public init(
headers: Headers = .init(),
statusCode: HTTPStatusCode = .processing,
body: ByteStream = .noStream,
reason: String? = nil) {
self.headers = headers
reason: String? = nil
) {
self._headers = headers
self._statusCode = statusCode
self.body = body
self._body = body
self.reason = reason
}

public init(headers: Headers = .init(), body: ByteStream, statusCode: HTTPStatusCode, reason: String? = nil) {
self.body = body
self._body = body
self._statusCode = statusCode
self.headers = headers
self._headers = headers
self.reason = reason
}

/**
Expand Down
15 changes: 0 additions & 15 deletions Sources/SmithyHTTPAPI/HTTPURLResponse.swift

This file was deleted.

95 changes: 63 additions & 32 deletions Sources/SmithyHTTPAPI/Headers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,28 @@
// SPDX-License-Identifier: Apache-2.0
//

public struct Headers: Sendable {
public var headers: [Header] = []
import class Foundation.NSRecursiveLock

public struct Headers: @unchecked Sendable {
private let lock = NSRecursiveLock()

private var _headers: [Header] = []
public var headers: [Header] {
get { access { $0 } }
set { mutate { $0 = newValue } }
}

private mutating func mutate(_ block: (inout [Header]) -> Void) {
lock.lock()
defer { lock.unlock() }
block(&_headers)
}

private func access<T>(_ block: ([Header]) throws -> T) rethrows -> T {
lock.lock()
defer { lock.unlock() }
return try block(_headers)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Above, the headers property is protected with exclusive access. Below, various mutating methods & accessors are modified to use exclusive access.


/// Creates an empty instance.
public init() {}
Expand Down Expand Up @@ -53,11 +73,13 @@ public struct Headers: Sendable {
/// - Parameters:
/// - header: The `Header` to be added or updated.
public mutating func add(_ header: Header) {
guard let index = headers.index(of: header.name) else {
headers.append(header)
return
mutate { headers in
guard let index = headers.index(of: header.name) else {
headers.append(header)
return
}
headers[index].value.append(contentsOf: header.value)
}
headers[index].value.append(contentsOf: header.value)
}

/// Case-insensitively updates the value of a `Header` by replacing the values of it or appends a `Header`
Expand All @@ -66,11 +88,13 @@ public struct Headers: Sendable {
/// - Parameters:
/// - header: The `Header` to be added or updated.
public mutating func update(_ header: Header) {
guard let index = headers.index(of: header.name) else {
headers.append(header)
return
mutate { headers in
guard let index = headers.index(of: header.name) else {
headers.append(header)
return
}
headers.replaceSubrange(index...index, with: [header])
}
headers.replaceSubrange(index...index, with: [header])
}

/// Case-insensitively updates the value of a `Header` by replacing the values of it or appends a `Header`
Expand All @@ -97,17 +121,20 @@ public struct Headers: Sendable {
///
/// - Parameters:
/// - headers: The `Headers` object.
public mutating func addAll(headers: Headers) {
self.headers.append(contentsOf: headers.headers)
public mutating func addAll(headers otherHeaders: Headers) {
mutate { headers in
headers.append(contentsOf: otherHeaders.headers)
}
}

/// Case-insensitively removes a `Header`, if it exists, from the instance.
///
/// - Parameter name: The name of the `HTTPHeader` to remove.
public mutating func remove(name: String) {
guard let index = headers.index(of: name) else { return }

headers.remove(at: index)
mutate { headers in
guard let index = headers.index(of: name) else { return }
headers.remove(at: index)
}
}

/// Case-insensitively find a header's values by name.
Expand All @@ -116,13 +143,14 @@ public struct Headers: Sendable {
///
/// - Returns: The values of the header, if they exist.
public func values(for name: String) -> [String]? {
guard let indices = headers.indices(of: name), !indices.isEmpty else { return nil }
var values = [String]()
for index in indices {
values.append(contentsOf: headers[index].value)
access { headers in
guard let indices = headers.indices(of: name), !indices.isEmpty else { return nil }
var values = [String]()
for index in indices {
values.append(contentsOf: headers[index].value)
}
return values
}

return values
}

/// Case-insensitively find a header's value by name.
Expand All @@ -131,29 +159,28 @@ public struct Headers: Sendable {
///
/// - Returns: The value of header as a comma delimited string, if it exists.
public func value(for name: String) -> String? {
guard let values = values(for: name) else {
return nil
}
guard let values = values(for: name) else { return nil }
return values.joined(separator: ",")
}

public func exists(name: String) -> Bool {
headers.index(of: name) != nil
access { $0.index(of: name) != nil }
}

/// The dictionary representation of all headers.
///
/// This representation does not preserve the current order of the instance.
public var dictionary: [String: [String]] {
let namesAndValues = headers.map { ($0.name, $0.value) }

return Dictionary(namesAndValues) { (first, last) -> [String] in
return first + last
access { headers in
let namesAndValues = headers.map { ($0.name, $0.value) }
return Dictionary(namesAndValues) { (first, last) -> [String] in
first + last
}
}
}

public var isEmpty: Bool {
return self.headers.isEmpty
access { $0.isEmpty }
}
}

Expand All @@ -164,14 +191,18 @@ extension Headers: Equatable {
/// - rhs: The second `Headers` to compare.
/// - Returns: `true` if the two values are equal irrespective of order, otherwise `false`.
public static func == (lhs: Headers, rhs: Headers) -> Bool {
return lhs.headers.sorted() == rhs.headers.sorted()
lhs.access { lhsHeaders in
rhs.access { rhsHeaders in
lhsHeaders.sorted() == rhsHeaders.sorted()
}
}
}
}

extension Headers: Hashable {

public func hash(into hasher: inout Hasher) {
hasher.combine(headers.sorted())
access { hasher.combine($0.sorted()) }
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,10 +215,11 @@ class StructureGenerator(

writer.writeAvailableAttribute(model, shape)
writer.openBlock(
"public struct \$struct.name:L: \$N, \$error.protocol:N, \$N, \$N {",
"public struct \$struct.name:L: \$N, \$error.protocol:N, \$N, \$N, \$N {",
ClientRuntimeTypes.Core.ModeledError,
ClientRuntimeTypes.Http.HttpError,
SwiftTypes.Error
SwiftTypes.Error,
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.

The Sendable protocol is added to all rendered errors.

)
.call { generateErrorStructMembers() }
.write("")
Expand All @@ -234,7 +235,7 @@ class StructureGenerator(
private fun generateErrorStructMembers() {
if (membersSortedByName.isNotEmpty()) {
writer.write("")
writer.openBlock("public struct Properties {", "}") {
writer.openBlock("public struct Properties: \$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.

Modeled errors' internal Properties structure (when rendered) is marked Sendable. No other code change required for sendability since all possible model struct properties are already Sendable.

membersSortedByName.forEach {
val (memberName, memberSymbol) = memberShapeDataContainer.getOrElse(it) { return@forEach }
writer.writeMemberDocs(model, it)
Expand Down
Loading