-
Notifications
You must be signed in to change notification settings - Fork 721
Strict concurrency for NIOPerformanceTester and NIOCrashTester #3167
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
Conversation
|
|
||
| final class NIOLockBenchmark: Benchmark { | ||
| final class NIOLockBenchmark: Benchmark, @unchecked Sendable { | ||
| // mutable state is protected by the lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit but couldn't we wrap opsDone in a NIOLockedValueBox instead and remove the @unchecked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a Sendable-cleanness perspective, yes. However, this is a benchmark for NIOLock, so using NIOLockedValueBox would somewhat defeat the point of the benchmark!
| return channel.pipeline.addHandler(self.serverHandler) | ||
| channel.eventLoop.makeCompletedFuture { | ||
| let serverHandler = ServerHandler(connectionEstablishedPromise, eventLoop: channel.eventLoop) | ||
| promise.succeed(NIOLoopBound(serverHandler, eventLoop: channel.eventLoop)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this promise succeeded if adding the handler to the pipeline failed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If adding the handler fails then the bootstrap will throw anyway so it doesn't really matter.
|
|
||
| extension UDPBenchmark { | ||
| final class EchoHandler: ChannelInboundHandler { | ||
| final class EchoHandler: ChannelInboundHandler, Sendable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this to be Sendable if we have a SendableView?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SendableView is on a different type.
No description provided.