-
Notifications
You must be signed in to change notification settings - Fork 1.1k
On Web, implement Send for EventLoopProxy
#2737
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4d4829d to
101bdbb
Compare
101bdbb to
1bc4774
Compare
EventLoopProxy now implements SendSend for EventLoopProxy
1bc4774 to
e49cb32
Compare
| match self.receiver.try_recv() { | ||
| Ok(event) => Poll::Ready(Ok(event)), | ||
| Err(TryRecvError::Empty) => { | ||
| *self.waker.lock().unwrap() = Some(cx.waker().clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use will_wake here in order to avoid unnecessarily cloning a waker twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know about this! I also noticed that neither futures or atomic-waker use this this optimization in their AtomicWaker implementation. I opened an issue here: smol-rs/atomic-waker#11.
In our case this won't be possible because to avoid a racing condition we would have to hold the lock, which can lead to blocking in a multi-threaded environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't you do:
{
let mut waker = self.waker.lock().unwrap();
if waker.map_or(true, |waker| !waker.will_wake(cx.waker())) {
*waker = Some(cx.waker().clone());
}
}in order to drop the lock once the temporary scope ends?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't concerned with how to drop the lock, but the fact that we hold it in the first place. The assumption in play here is that if you only assign a value without any checks in-between, optimizations can prevent this to block the main thread because it won't yield in-between a simple assignment. Not really a provable assumption, nor one I'm certain about, but it's what we have right now without using AtomicWaker.
If the the worker yields between holding the lock and comparing and cloning the Waker, we are gonna have a problem.
|
|
||
| pub struct AsyncSender<T: 'static> { | ||
| sender: Sender<T>, | ||
| waker: Arc<Mutex<Option<Waker>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Mutex available on WASM? I always forget what synchronization primitives panic on block for non-atomic WASM or not. Although, it looks like that, in single threaded cases, it shouldn't block. Is this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
Mutexavailable on WASM?
Yes.
Although, it looks like that, in single threaded cases, it shouldn't block. Is this right?
For single threaded cases it shouldn't block indeed, but it can for multi-threaded cases. Considering we are just storing a single value and not holding a lock in practice it's not a problem.
I would still like to propose that we don't use a Mutex here and use AtomicWaker instead. The crate has no dependencies and is really small.
If the dependency is still a concern, I would like to point to my suggestion above: hiding the Send support behind the +atomics target feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would support using atomic-waker, but I'd ask the rest of the maintainers first.
|
CI fails on iOS because of |
Yeah, the iOS CI check does that sometimes. It's not your fault |
752ae33 to
f579aee
Compare
|
Closing in favor of #2740. |
CHANGELOG.mdif knowledge of this change could be valuable to usersSee #2294 for previous discussion on this.
This adjusts
EventLoopProxyon Web to beSend, like other platforms.Because the
EventLoopitself is notSend, the way this is solved is by using a channel that receives messages in the background and redirects them to theEventLoop, with the help ofwasm_bindgen_futures::spawn_local(), adding a new dependency, even if fairly minor.Avoiding the
wasm-bindgen-futuresis kind of impossible because we need async support, but I did build our own async channel with the help of a simpleMutex.I have some additional ideas in mind:
We could avoid the additional dependency by saying that we only support
Sendwhen you are usingtarget_feature = "atomics"and otherwise fall back to what we were doing until now. This would add a bunch ofcfgs inside the code.With this PR we will always use the channel, even when we are on the main thread. This could be avoided by using something similar like on MacOS with
MainThreadSafe. This would also improve the performance overhead this PR introduces by circumventing the channel when not needed.If we don't want to import a big crate like
async-channelwe could at least importatomic-waker, which has no dependencies and is quite tiny, removing theMutex.I would like to give the same treatment to
Window, which also doesn't implementSendin spite of all other targets having it.I adjusted the web example to send custom user events to try this out, but it's written in a way to also be able to compile on other platforms, which adds a lot of
cfgs everywhere, not sure if I should add it here:Adjusted Example
Partially replaces #2294.
Fixes #2592.