NIOSendableBox: allow off-loop initialisation iff Value is Sendable #2753
Conversation
weissi
force-pushed
the
jw-sendable-box-init
branch
from
fafd1bb
to
80318e0
Compare
| /// Contrary to ``init(_:eventLoop:)``, this method can be called off `eventLoop` because we know that `value` is ``Sendable``. | ||
| /// So we don't need to protect `value` itself, we just need to protect the ``NIOLoopBoundBox`` against mutations which we do because the ``value`` | ||
| /// accessors are checking that we're on `eventLoop`. | ||
| public static func makeBoxSendingValue( |
There was a problem hiding this comment.
I find it weird to use a factory method here personally. I would prefer using an init here. I also see little value in having this constrained to Sendable since Sendable types can freely be shared between threads.
I expected a spelling like this
init(value: sending Value, eventLoop: any EventLoop) {}
This would allow moving an non-Sendable value into an EL's isolation region which is dynamically enforced by the LoopBoundBox.
There was a problem hiding this comment.
- factory is to get "more words", the
inits are unconstrained but have to run oneventLoop. And themakeBoxSendingValueones can be run off the EL with various constraints (eitherSendableorsending) sendingisn't ready yet (Give NIOLoopBoundBox asendingoff-EL initialiser once compiler is ready #2754)- there's a huge value for
Sendablevalues. It's not about sending the values around,NIOLoopBoundBoxis about mutating the values. Typically this is for thread-safe network clients/servers that have a state machine which only gets accessed on a particular EL. So current:var state: State // only access on self.eventLoop; new:let state: NIOLoopBound<State>
There was a problem hiding this comment.
I understand your goal but IMO NIOLoopBoundBox with an already Sendable value does not make sense. What you want here is just a plain old Box<T> that is conditionally Sendable on T. If the boxed value is already Sendable then the loop enforcement adds no value since the type is already thread safe. The storage might not i.e. the var but the only thing needed for that is to make it a let through adding reference semantics. let boxed = Box<MySendableType> is totally safe.
There was a problem hiding this comment.
That's not accurate.
class Box<T> { var value: T }cannot be Sendable (without synchronisation), you get mutation and sharing.class NIOLoopBoundBox<T> { var value: T /* checks EL on all accesses */ }is alwaysSendablebecause it checks that all accesses are on the same EL. So you get mutation but no sharing
There was a problem hiding this comment.
I've got no objection to the PR in its current state, but I'm not going to approve because I'd like to see @FranzBusch and @weissi get aligned here.
There was a problem hiding this comment.
I am fine with taking this as is.
|
|
@swift-nio-bot test failed |
|
@swift-nio-bot test this please |
* commit 'fc79798d5a150d61361a27ce0c51169b889e23de': NIOSendableBox: allow off-loop initialisation iff Value is Sendable (apple#2753) Throw an appropriate error from the writer when the channel closed (apple#2744) put snippet code inside @available function (apple#2750) fix link to NIOFileSystem from NIO index page (apple#2747) convert the NIOFileSystem example code to a Snippet (apple#2746) Silence warning about missing include in macOS builds (apple#2741) Correctly mark 304 as not having a response body (apple#2737) Update availability guard (apple#2739) Add API for setting last accessed and last modified file times (apple#2735) Add a fallback path if renameat2 fails (apple#2733) Release file handles back to caller on failure to take ownership (apple#2715) Add a version of 'write' for 'ByteBuffer' (apple#2730) Imrprove rename error (apple#2731) Remove storage indirection for FileSystemError (apple#2726) testSimpleMPTCP should not fail for ENOPROTOOPT (apple#2725) Fix race in TCPThroughputBenchmark (apple#2724)
Motivation:
NIOLoopBoundandNIOLoopBoundBoxare important tools for making mutable typesSendable.The power of
NIOLoopBoundBoxis that it allows correct mutation, whilst remainingSendablewithout locks and without having to use@uncheckedif the mutable class makes sure to have all accesses (reading & writing) run on one particularEventLoop. This is safe asEventLoops guarantee sequentially consistent memory order across executions of different work items/events. TypicallyEventLoops achieve this by just being thread-bound.These types are well used already but there's a small and common pattern which is safe but unsupported as of yet:
Initialise a
NIOLoopBoundBoxwith aSendablevalue off the loop. All further accesses (reads & writes) however happen on the loop. That's safe because of Swift's Definitive Initialisation (DI) butNIOLoopBoundBoxdidn't support this pattern (apart frommakeEmptyBoxwhich always initialises withnil).Modifications:
Sendablevalues to be provided whilst creating the type off loop.Result:
Sendablewithout using `@unchecked.