Enforce Sendability #204
Enforce Sendability #204
Conversation
| public class NIOPCAPRingBuffer { | ||
| private var pcapFragments: CircularBuffer<ByteBuffer> | ||
| private var pcapCurrentBytes: Int | ||
| public final class NIOPCAPRingBuffer: Sendable { |
There was a problem hiding this comment.
Is this change necessary? The type wasn't thread-safe before, which suggests it was bound to a specific isolation context.
There was a problem hiding this comment.
This is a good question. This class is only used in tests by us (although it's public API).
I was trying to get rid of a warning where a NIOPCAPRingBuffer is captured in an ELF/map chained after a call to write on ChannelHandlerContext (the test in question is in TriggerOnCumulativeSizeHandler/TriggerOnCumulativeSizeHandler/write):
let boundedSink = NIOLoopBound(self.sink, eventLoop: context.eventLoop)
context.write(data).map { [pcapRingBuffer] in
boundedSink.value(captureBytes(ringBuffer: pcapRingBuffer))
}.cascade(to: promise)This piece of code also seems like a somewhat sensible pattern to do as a client when you're trying to debug something (which is why this class exists as public API, I assume). I'm not sure how to refactor this to get rid of the warning without making the class Sendable, as pcapRingBuffer is modified as a result of the call to context.write(). This means I can't call captureBytes(ringBuffer:) outside of the closure, as we'd trigger the PCAP at the wrong time (causing the wrong behaviour and assertion failures).
I'm open to refactor suggestions if we want to avoid making this Sendable. Having said that, is there any reason why you would suggest avoiding it, Cory? Or would we be better off making these changes?
| private let pcapLock = NIOLock() | ||
| private var pcapFragments: CircularBuffer<ByteBuffer> | ||
| private var pcapCurrentBytes: Int |
There was a problem hiding this comment.
Rather than using a lock should we create a State struct and stick that in a locked-value-box?
| @@ -39,122 +39,135 @@ internal struct NFS3FileSystemInvoker<FS: NFS3FileSystemNoAuth, Sink: NFS3FileSy | |||
| case .mountNull: | |||
| self.sink.sendSuccessfulReply(.mountNull, call: callMessage) | |||
| case .mount(let call): | |||
| let boundedSink = NIOLoopBound(self.sink, eventLoop: self.eventLoop) | |||
There was a problem hiding this comment.
What guarantees that we're on the correct event-loop here?
This PR fixes Sendability warnings present when enabling strict concurrency checking.
Motivation:
We want to make sure
swift-nio-extrasis free of concurrency bugs, and thus removing Sendability warnings is crucial.Modifications:
Some types have been made Sendable, and in other places either patterns have been changed to ensure we're not crossing concurrency domains, or values have been wrapped with
NIOLoopBoundwhen we know we're on a specific EL.Result:
No more warnings in
swift-nio-extras.