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

chore: Make modeled errors Sendable #883

merged 5 commits into from
Dec 18, 2024

Conversation

jbelkins
Copy link
Contributor

@jbelkins jbelkins commented Dec 10, 2024

Description of changes

Swift Sendable conformance is added to all modeled errors. There is no change to public APIs except the addition of the Sendable protocol to various types.

  • Headers is made @unchecked Sendable by placing exclusive locking on its properties.
  • HTTPResponse is made @unchecked Sendable by placing exclusive locking on its properties as well.
  • Sendable conformance is added to code-generated error structures and their internal Properties.

Also:

  • The HTTPURLResponse protocol, which is unused and scoped internal, is deleted.

Scope

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@@ -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.

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.

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.

}

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 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 struct Properties {
public struct Properties: Swift.Sendable {
/// This is documentation about the member.
public internal(set) var baz: Swift.Int? = nil
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 above codegen test expectation is an example of how generated errors are changed.

@jbelkins jbelkins marked this pull request as ready for review December 17, 2024 22:00
Copy link
Contributor

@sichanyoo sichanyoo left a comment

Choose a reason for hiding this comment

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

LGTM

@jbelkins jbelkins merged commit c1f3dea into main Dec 18, 2024
27 checks passed
@jbelkins jbelkins deleted the jbe/20241209_sendable branch December 18, 2024 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants