wayland-client: Add tokio feature for AsyncEventQueue #702
Conversation
Gated on the `tokio` feature flag
| .map(NonZeroUsize::new) | ||
| } | ||
|
|
||
| /// Asynchronous version of [`EventQueue::blocking_dispatch`] that uses tokio's fd-based |
There was a problem hiding this comment.
Doc comment needs adjusting due to API changes from reference -> owned version of EventQueue member.
|
Why such API needs special treatment for tokio? Can't the approach calloop-wayland-source is doing be adopted just for tokio in a separate crate? Like in the end of the day you should just add Like this API just looks like a candidate for external crate, otherwise we'll aggregate all bits for all async runtimes. |
| let mut guard = self.afd.readable_mut().await | ||
| .map_err(|e| DispatchError::Backend(WaylandError::from(e)))?; | ||
| let lock = if let Some(v) = self.queue.prepare_read() { | ||
| v | ||
| } else { | ||
| if let Some(v) = self.dispatch_pending(data)? { | ||
| return Ok(v.get()); | ||
| } else { | ||
| continue; | ||
| } | ||
| }; | ||
| let ret = match lock.read() { | ||
| Ok(..) => Some(self.queue.dispatch_pending(data)), | ||
| Err(ref e) if e.would_block() => { | ||
| guard.clear_ready_matching(Ready::READABLE); | ||
| None | ||
| }, | ||
| Err(e) => Some(Err(DispatchError::from(e))), | ||
| }; |
There was a problem hiding this comment.
This implementation is not correct, and it may be difficult to be made correct given the tokio paradigm.
The way prepare_read() work (and this is a constraint coming from libwayland itself), it must be invoked before waiting for the FD to become readable. Not doing so will cause issues in a multithreaded context, which some threads never being made aware that events are available to process.
At the same time, because the guard returned by prepare_read() behaves like a semaphore, you also need to ensure that never more than one such guard is created on a single thread (otherwise this will cause a deadlock), and that the guard is as short-lived as possible, ie it should be be created just before starting to wait on epoll, and consume just after epoll has returned (by invoking .read() or by dropping it). Having a thread hold onto such a guard while doing other stuff will block all other threads from reading from the socket, and may in turn cause deadlocks on the app at large.
I'm bringing these points not as a "worst case scenario" hypothetical, those are issues that we have already encountered previously.
There was a problem hiding this comment.
The way
prepare_read()work (and this is a constraint coming from libwayland itself), it must be invoked before waiting for the FD to become readable. Not doing so will cause issues in a multithreaded context, which some threads never being made aware that events are available to process.
Unfortunate. I had done some super basic testing and it had seemed okay, but that makes sense that that requirement would completely negate this implementation.
Thanks for the info; definitely the reason I listed this implementation as one of the things that needed looked over from someone more intimately familiar than me.
This implementation is not correct
Hence why I said it could use a looksie by someone more experienced than me! Good catch. Not something I would have thought of, or perhaps ever caught despite thinking I'd read upstream's docs well enough 🤣
Thanks for the feedback and fixing my eminently broken project that spawned this request! 😄
I'll take a look at what they're doing over there; thanks.
Well, at that point, you're being opinionated about I initially chose this approach due to needing it for another project, and only adapted for upstream when I noticed that both examples and an API were missing. The more I think about it, though, the more I sort of agree that perhaps the real "solution" here would be some example implementation, rather than a new API in and of itself.
I kind-of agree, given the While this If you were to try to solve that latter problem, the lack of either an async-capable API, or example integration, it sounds like you'd approach it from the Thanks again for the feedback; like I said, simply just trying to ease someone else's pain so they don't have to dig on this as much comparatively; as a user, it definitely feels like there's a gap in either an example runtime integration, or (less likely), some kind of API like this implementation. I'll play around w/ what one can do from the
Definitely agree that if there's an ability to provide either some kind of generic API or example implementation, that it would be a much better approach than this. Wasn't really aware one could undercut the runtime w/r/t Thanks for the time/comments. |
I used mio as an example, since that's what tokio uses, in the end of the day you want to add fd into Basically look what https://github.com/Smithay/calloop-wayland-source is doing and then map stuff into tokio. |
Okay that makes more sense. I'll look through the calloop implementation, and perhaps the "issue" is more the lack of a simple link in the doc comments to that implementation to aid in users finding it, rather than anything that would be beneficial to add here. |
|
Since whatever follows won't be anything akin to this, I'll close for now. |
|
I tried my hand at async for client dispatch some time ago but haven't really publish the surrounding code yet. With async we have no guarantees what thread or when future is polled (although typically the future for Wayland would have it's wakers sent off it OS polling apis. Essentially with the prepare_read API there is a risk of deadlocks if you hold the read guard across await points. The most simple way to deal with this is to just spawn a thread to read from the socket and then wake up any tasks that have event queues that need to read. The code linked (although do audit it as it may have some bugs) uses the smol family of crates (because these are easier to integrate into multiple different executors), though the same principles should be applicable with tokio: https://gist.github.com/i509VCB/727f8eb2f567ef108740859436ee765e |
This would add a
tokiofeature to thewayland-clientcrate, providing asynchronous versions of the following functions for anEventQueue.I came across a desire for this integration while writing a single-threaded client, and needed to integrate events coming from other
tokiosources.I cannot find it now, but I swear I also once saw an issue asking how to integrate the
_pollEventQueueAPI intotokio.Known Blocking Problems
Known Lesser Issues
AsyncEventQueueretains a reference toConnectionspecifically to appease lifetime req's ofAsyncFdso that theAsyncFdcan be long-lived. This should be avoidable.simple_window_asyncexample only currently uses onetokioevent source, making it somewhat meaninglesssimple_windowandsimple_window_asyncexamples, but usinginclude!(..)macros to de-duplicate caused issues with thewayland-scannerproc macrosdispatch_singleimplementation could use review to ensure thatEventQueue::flushcalls are necessary and well-placeddispatch_singleimplementation is broken due to expectations ofprepare_read()being that it's called before the fd is readableDiscussion Points
wayland-clientwith afeatureflag as it is in this implementation, or would another crate make more sense? I personally think the former, but could see both.dispatch_singleAPI onAsyncEventQueuemake sense as a low-enough level API or does this feel more like a reference implementation that deserves lower level APIs?tokiothat offering a feature flag isn't too opinionated of a thing to do, given the # of folks who may like to have an API like this w/o thinking through theAsyncFdusage themselvesSorry for the messy writeup, but I keep saying I'd break this out of the project I originally made it for, and submit it for upstream, and finally decided to do it even if it requires some discussion and a few edits (particularly around documentation) prior to being mergeable.