-
Notifications
You must be signed in to change notification settings - Fork 927
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
On Web, implement Send
for EventLoopProxy
#2737
Conversation
4d4829d
to
101bdbb
Compare
101bdbb
to
1bc4774
Compare
EventLoopProxy
now implements Send
Send
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
Mutex
available 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.md
if knowledge of this change could be valuable to usersSee #2294 for previous discussion on this.
This adjusts
EventLoopProxy
on Web to beSend
, like other platforms.Because the
EventLoop
itself 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-futures
is 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
Send
when you are usingtarget_feature = "atomics"
and otherwise fall back to what we were doing until now. This would add a bunch ofcfg
s 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-channel
we 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 implementSend
in 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
cfg
s everywhere, not sure if I should add it here:Adjusted Example
Partially replaces #2294.
Fixes #2592.