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

Adopt Sendable #621

Merged
merged 1 commit into from
Aug 25, 2022
Merged

Adopt Sendable #621

merged 1 commit into from
Aug 25, 2022

Conversation

dnadoba
Copy link
Collaborator

@dnadoba dnadoba commented Aug 24, 2022

Updates swift-nio, swift-nio-ssl and swift-nio-extras to the latest released version to use the latest Sendable changes in these libraries.

@dnadoba dnadoba added the semver/minor For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Aug 24, 2022
@dnadoba dnadoba force-pushed the dn-sendable branch 9 times, most recently from 647569b to 7d1ca5c Compare August 24, 2022 12:07
@@ -647,4 +647,47 @@ extension AsyncSequence where Element == ByteBuffer {
}
}
}

struct AnySendableSequence<Element>: @unchecked Sendable {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have used AnySequence and AnyCollection to force a specific overload of the HTTPClientRequest.Body.bytes method. The Sequence/Collection passed to these methods need to be Sendable now but AnySequence and AnyCollection are not Sendable. AnySendableSequence and AnySendableCollection are simple wrappers of their non-Sendable counterparts.

@@ -541,16 +541,26 @@ final class TransactionTests: XCTestCase {
// tasks. Since we want to wait for things to happen in tests, we need to `async let`, which creates
// implicit tasks. Therefore we need to wrap our iterator struct.
@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
actor SharedIterator<Iterator: AsyncIteratorProtocol> {
private var iterator: Iterator
actor SharedIterator<Wrapped: AsyncSequence> where Wrapped.Element: Sendable {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wrapped.Element: Sendable constraint necessary in Swift 5.8 because return types of actor isolated functions need to be Sendable since SE-0338 and Wrapped.Element is the return type of next().


init(_ iterator: Iterator) {
self.iterator = iterator
init(_ sequence: Wrapped) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Iterator is now created in the actor instead of outside the actor. This is necessary because AsyncIterators are often non-Sendable but AsyncSequences themself are often Sendable. Passing in an non-Sendable value produces a warning with Swift 5.8 (Swift Development Snapshot 2022-08-18 (a)) e.g.:

actor Actor<Element> {
    init(_ element: Element) {}
}
class NonSendableClass {}

let nonSendable = NonSendableClass()
_ = Actor(nonSendable) // warning: Non-sendable type 'NonSendableClass' passed in call to nonisolated initializer 'init(_:)' cannot cross actor boundary

I'm not 100% sure if this warning is correct and need to think a bit more about this warning. However, the change doesn't hurt either.

Comment on lines +553 to +554
precondition(self.nextCallInProgress == false)
self.nextCallInProgress = true
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This construct is quite unsafe and doesn't support reentrancy. nextCallInProgress makes it a bit safe and will crash if next is called while another call to next is still in progress.

@dnadoba dnadoba marked this pull request as ready for review August 24, 2022 21:35
@dnadoba
Copy link
Collaborator Author

dnadoba commented Aug 24, 2022

API breakages are all false positives. Adding a Sendable requirement with @preconcurrency is not source breaking.

18:41:47 15 breaking changes detected in AsyncHTTPClient:
18:41:47   💔 API breakage: func HTTPClientRequest.Body.bytes(_:) has generic signature change from <Bytes where Bytes : Swift.RandomAccessCollection, Bytes.Element == Swift.UInt8> to <Bytes where Bytes : Swift.RandomAccessCollection, Bytes : Swift.Sendable, Bytes.Element == Swift.UInt8>
18:41:47   💔 API breakage: func HTTPClientRequest.Body.bytes(_:) is now with @preconcurrency
18:41:47   💔 API breakage: func HTTPClientRequest.Body.bytes(_:length:) has generic signature change from <Bytes where Bytes : Swift.Sequence, Bytes.Element == Swift.UInt8> to <Bytes where Bytes : Swift.Sendable, Bytes : Swift.Sequence, Bytes.Element == Swift.UInt8>
18:41:47   💔 API breakage: func HTTPClientRequest.Body.bytes(_:length:) is now with @preconcurrency
18:41:47   💔 API breakage: func HTTPClientRequest.Body.bytes(_:length:) has generic signature change from <Bytes where Bytes : Swift.Collection, Bytes.Element == Swift.UInt8> to <Bytes where Bytes : Swift.Collection, Bytes : Swift.Sendable, Bytes.Element == Swift.UInt8>
18:41:47   💔 API breakage: func HTTPClientRequest.Body.bytes(_:length:) is now with @preconcurrency
18:41:47   💔 API breakage: func HTTPClientRequest.Body.stream(_:length:) has generic signature change from <SequenceOfBytes where SequenceOfBytes : _Concurrency.AsyncSequence, SequenceOfBytes.Element == NIOCore.ByteBuffer> to <SequenceOfBytes where SequenceOfBytes : Swift.Sendable, SequenceOfBytes : _Concurrency.AsyncSequence, SequenceOfBytes.Element == NIOCore.ByteBuffer>
18:41:47   💔 API breakage: func HTTPClientRequest.Body.stream(_:length:) is now with @preconcurrency
18:41:47   💔 API breakage: func HTTPClientRequest.Body.stream(_:length:) has generic signature change from <Bytes where Bytes : _Concurrency.AsyncSequence, Bytes.Element == Swift.UInt8> to <Bytes where Bytes : Swift.Sendable, Bytes : _Concurrency.AsyncSequence, Bytes.Element == Swift.UInt8>
18:41:47   💔 API breakage: func HTTPClientRequest.Body.stream(_:length:) is now with @preconcurrency
18:41:47   💔 API breakage: func HTTPClient.shutdown(queue:_:) is now with @preconcurrency
18:41:47   💔 API breakage: func HTTPClient.Body.stream(length:_:) is now with @preconcurrency
18:41:47   💔 API breakage: func HTTPClient.Body.bytes(_:) has generic signature change from <Bytes where Bytes : Swift.RandomAccessCollection, Bytes.Element == Swift.UInt8> to <Bytes where Bytes : Swift.RandomAccessCollection, Bytes : Swift.Sendable, Bytes.Element == Swift.UInt8>
18:41:47   💔 API breakage: func HTTPClient.Body.bytes(_:) is now with @preconcurrency
18:41:47   💔 API breakage: constructor HTTPClientCopyingDelegate.init(chunkHandler:) is now with @preconcurrency

@dnadoba dnadoba merged commit c3c90aa into swift-server:main Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/minor For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants