swift-nio
swift-nio copied to clipboard
apple
Add NIOAsyncWebSocketServerUpgrader
One for discussions, adds a new async version of NIOWebSocketServerUpgrader. Thoughts? This allows the upgrader to be async/await all the way through, without needing to jump between event loops inside the class, or having optional async upgraders etc and push the boundary down to the channel handlers
At some point, somewhere, something has to thunk between the async model and the event loop model. I think it’s fine to propose completely reimplementing upgraders in an async pattern, such that we’d end up with this, but we’ll need to add a bridge protocol. The patch as written won’t compile, because it conforms to the Future-based version of the protocol.
So I think this approach is generally fine, but we should first make the HTTP protocol upgrader API async-capable, and then we can deal with reimplementing the websocket upgrader.
@Lukasa so we've hit this issue in Vapor in that we can't make protocols async compliant without breaking changes. The way we've done it is to add async versions of the protocol that conform to the non-async version to work around it, that make sense? I've done that here
I don't think the CI failure is me now
The nightly build is likely broken because of the compiler crasher noted in SR-15040 (can't say for sure since the error log here doesn't show any compiler output), which will probably be fixed by the next nightly snapshot, that will include the fix from apple/swift#39582.
@Lukasa cool, last thing I'll do is duplicate the tests and use the async upgrader then we should be good to go
@Lukasa I'm struggling to get the tests to pass. Even a simple testRequiresVersion13 is triggering an error on server shutdown instead of when writing the buffer to the server. I'm guessing my upgrade handler is handler an error wrong but that's pushing my NIO knowledge
Honestly, I don't think attempting to jerry-rig EmbeddedChannel to be suitable for testing async functions is going to be likely to work. I think we'll need an alternative to EmbeddedChannel that is an actor, and can safely execute async functions and wait for their completion.
This sounds like a fun project. Mind if I take a crack at it? Have you had any thoughts about the implementation that you'd be able to share?
Sure thing, go for it.
My current thought right now is that the closes analog to an EmbeddedChannel is a channel that owns a private DispatchQueue, and that dispatches all operations onto that queue. This allows it to have the appropriate scheduling and isolation logic. Otherwise it would be the same as EmbeddedChannel (e.g. doesn't use a real clock, doesn't do things unless told).
In an ideal world it would be an actor but if it's an actor it's not possible to answer the question "am I on the event loop" in a coherent way. That's pretty unfortunate, and ends up totally disqualifying this from being a proper actor.
Is there any utility in looking at an actor-based event loop implementation? It seems like that's the sticking point at the moment.
Aside from that, I feel like it ought to be possible to use unsafe continuations to dispatch work onto an event loop instance in an async-friendly fashion. It's been a while though, and maybe there are some special cases I don't recall where the channel code requires synchronicity from the event loop in respect to other channels on the same loop?
Is there any utility in looking at an actor-based event loop implementation? It seems like that's the sticking point at the moment.
We're unlikely to be really willing to do that until we can use custom executors. The principal issue is that we want to be able to block threads on selectors, and actors cannot do that (they can't block in the cooperative thread pool). A complete actor based event loop that didn't do this would need a complete parallel I/O system that involved a separate I/O thread, and the performance implications of that are fairly alarming.
Aside from that, I feel like it ought to be possible to use unsafe continuations to dispatch work onto an event loop instance in an async-friendly fashion.
Yup, you can do that, and NIO already provides a bunch of helpers. The thing we need here is a replacement for EmbeddedEventLoop and EmbeddedChannel. The issue with these types is that they are embedded directly in the creating thread: they do not have a parallel thread context of their own. As a result, we can't use continuations to re-establish a thread context there.
This specific use-case would be a replacement for EmbeddedEventLoop that still allows control over when and how I/O happens, but that is tied to a specific actor. This is not a fully-fledged actor event loop, but it would be pretty close.
@Lukasa is the NIOAsyncTestingChannel supposed to be a drop-in replacement for EmbeddedChannel?
In the new tests I'm running
try client.writeString(upgradeRequest).wait()
and it's never returning
@0xTim Not a perfect one, because it's impossible to write a perfect one, so it's a next-best one. What does writeString do in that code?