Add scheduleCallback APIs to NIOIsolatedEventLoop
#3263
Conversation
Sources/NIOCore/EventLoop.swift
Outdated
| @discardableResult | ||
| func _scheduleCallbackIsolatedUnsafeUnchecked( | ||
| at deadline: NIODeadline, | ||
| handler: some NIOScheduledCallbackHandler | ||
| ) throws -> NIOScheduledCallback | ||
|
|
||
| @discardableResult | ||
| func _scheduleCallbackIsolatedUnsafeUnchecked( | ||
| in amount: TimeAmount, | ||
| handler: some NIOScheduledCallbackHandler | ||
| ) throws -> NIOScheduledCallback |
There was a problem hiding this comment.
@Lukasa do we need those additional hooks on the EL? What was the reasoning behind adding the _scheduleCallbackIsolatedUnsafeUnchecked?
There was a problem hiding this comment.
We needed it because the existing EL hooks require Sendable, and they require a specific type. So it isn't possible to pass a T through them that isn't Sendable, because wrapping it into a NIOLoopBound makes it not a T anymore. In this case, a NIOLoopBound<some (NIOScheduledCallbackHandler & Sendable)> isn't a some (NIOScheduledCallbackHandler & Sendable), so it cannot be passed through there.
Wrapping things in a sufficiently large number of loop bounds can work, but performs poorly, and we don't want to pessimise the performance of the on-loop case. That's what we want to work best.
There was a problem hiding this comment.
Great, if I understand you correctly adding the above methods is our intend outcome.
There was a problem hiding this comment.
Does this mean, we want to work with our own unsafe transfer method in this PR, so that we don't use NIOLoopBound at all? To work around the preconditions?
There was a problem hiding this comment.
Cool, so we do want to use assertions (and therefore the ...UnsafeUnchecked method) rather than the NIOLoopBound which provides a precondition
There was a problem hiding this comment.
Oh I think we wrote that at the same time 😄
There was a problem hiding this comment.
Yeah, and we should also implement a version that doesn't require this fallback for SelectableEventLoop, EmbeddedEventLoop, and AsyncTestingEventLoop.
There was a problem hiding this comment.
Cool, done. 97deac6 Should I leave those changes for a different PR?
There was a problem hiding this comment.
Sounds reasonable to me.
There was a problem hiding this comment.
Thanks so much for the progress so far and for tackling this issue! @Lukasa can you please also give this an early look?
Sources/NIOCore/EventLoop.swift
Outdated
| @discardableResult | ||
| func _scheduleCallbackIsolatedUnsafeUnchecked( | ||
| at deadline: NIODeadline, | ||
| handler: some NIOScheduledCallbackHandler | ||
| ) throws -> NIOScheduledCallback | ||
|
|
||
| @discardableResult | ||
| func _scheduleCallbackIsolatedUnsafeUnchecked( | ||
| in amount: TimeAmount, | ||
| handler: some NIOScheduledCallbackHandler | ||
| ) throws -> NIOScheduledCallback |
There was a problem hiding this comment.
We needed it because the existing EL hooks require Sendable, and they require a specific type. So it isn't possible to pass a T through them that isn't Sendable, because wrapping it into a NIOLoopBound makes it not a T anymore. In this case, a NIOLoopBound<some (NIOScheduledCallbackHandler & Sendable)> isn't a some (NIOScheduledCallbackHandler & Sendable), so it cannot be passed through there.
Wrapping things in a sufficiently large number of loop bounds can work, but performs poorly, and we don't want to pessimise the performance of the on-loop case. That's what we want to work best.
|
Okay so I think this is ready for a proper review |
| @@ -170,3 +170,26 @@ extension EventLoop { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| @usableFromInline | |||
| struct UnsafeUncheckedScheduledCallbackHandlerWrapper<Handler: NIOScheduledCallbackHandler>: | |||
There was a problem hiding this comment.
I don't think this needs to be unsafe. Instead, let's implement this in terms of NIOLoopBound. That way we preserve the logical semantics, and avoid an @unchecked Sendable.
There was a problem hiding this comment.
Didn't we decide on using assertions rather than preconditions a few messages above?
There was a problem hiding this comment.
In the unchecked fast path, yes. But the slower fallback is just that: slower. As a result, we can safely re-use things we already have, and avoid proliferating @unchecked Sendable.
In general I have a plan to increasingly remove the @unchecked Sendables in NIO because there are far too many, and they can be very damaging. So wherever we can use an existing spelling for this, I'd prefer we did.
Fixes #3262 by adding the missing APIs.
Motivation:
As explained by the issue linked above, having a non-
Sendable-requiring variant of the newscheduleCallbackAPIs onNIOIsolatedEventLoopcan be useful since we're not always dealing withSendabletypes.Modifications:
This adds the required
scheduleCallbackAPIs toNIOIsolatedEventLoopby wrapping the non-SendableNIOScheduleCallbackHandlerin aNIOLoopBound-based handler.Result:
The two
scheduleCallbacks can be used onNIOIsolatedEventLooptoo.