Deal with Sendable and clean up package in general#158
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #158 +/- ##
==========================================
- Coverage 82.85% 82.39% -0.46%
==========================================
Files 6 6
Lines 659 659
==========================================
- Hits 546 543 -3
- Misses 113 116 +3
🚀 New features to boost your workflow:
|
…f the SSL helpers
|
5.10 CI failure is caused by NIO, see apple/swift-nio#3223 |
|
|
||
| func testCreateNewELGAndShutdown() throws { | ||
| let client = WebSocketClient(eventLoopGroupProvider: .createNew) | ||
| let client = WebSocketClient(eventLoopGroupProvider: .shared(.singletonMultiThreadedEventLoopGroup)) |
There was a problem hiding this comment.
Did this have a warning or something? because the test name doesn't quite match the change you made.
There was a problem hiding this comment.
.createNew is deprecated and I wasn't paying attention to the test name 🤦♀️
| import NIOHTTP1 | ||
|
|
||
| final class HTTPUpgradeRequestHandler: ChannelInboundHandler, RemovableChannelHandler { | ||
| final class HTTPUpgradeRequestHandler: ChannelInboundHandler, RemovableChannelHandler, Sendable { |
There was a problem hiding this comment.
Why does this need to be Sendable?
There was a problem hiding this comment.
Because I didn't think of using NIOLoopBound first. I'll change it.
| let responseEncoder = HTTPResponseEncoder() | ||
| let requestDecoder = ByteToMessageHandler(HTTPRequestDecoder(leftOverBytesStrategy: .forwardBytes)) | ||
| let proxySimulator = HTTPProxySimulator(promise: promise, config: proxyConfig, expectedAuthorization: expectedAuthorization) | ||
| let responseEncoder = NIOLoopBound(HTTPResponseEncoder(), eventLoop: channel.eventLoop) |
There was a problem hiding this comment.
It's tests so doesn't matter as much but why the loop bound dance here? We're @unchecked Sendable and @preconcurrency importing the types. I would have assumed one of the 3 approaches would be enough
There was a problem hiding this comment.
Because futures are now Strict Concurrency-correct and everything you capture in a closure has to be Sendable in ways that @preconcurrency would only silence if you applied it to the entire NIOCore import.
0xTim
left a comment
0xTim
left a comment
There was a problem hiding this comment.
Nice to see the tests passing again! That should at least unblock other PRs
3 participants
These changes are now available in 2.16.0
Covers fixing a bunch of
Sendablethings and does the usual round of package cleanup - bump the Swift minimum to 5.10, update the CI and the README, and so forth. Also fixes up some issues in the tests.