Skip to content
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

It is a foot-gun that EventLoopProxy is !Sync on macOS #3448

Closed
gretchenfrage opened this issue Feb 1, 2024 · 3 comments · Fixed by #3449
Closed

It is a foot-gun that EventLoopProxy is !Sync on macOS #3448

gretchenfrage opened this issue Feb 1, 2024 · 3 comments · Fixed by #3449
Labels
DS - ios DS - macos DS - web F - question There's no such thing as a stupid one S - api Design and usability

Comments

@gretchenfrage
Copy link

Hi all, thank you for your consideration. Someone previously created #3220 on this subject, but then closed their own issue when they found that there actually is an obstacle to making EventLoopProxy Sync on macOS. However, I'm creating this issue for further discussion because I think it is unfortunate that the API is as such.

EventLoopProxy being Sync only on non-macOS platforms makes it so that one can write code with winit that doesn't seem to be using any platform-specific features but nonetheless fails to compile on macOS while compiling fine on other platforms, which I think goes against winit's generally very nice goals and behaviors in terms of cross-platform abstraction.

Some possible ways I can imagine dealing with this include:

  • Make EventLoopProxy always !Sync
  • Make EventLoopProxy Sync on macOS by wrapping its internals in a mutex
  • Make EventLoopProxy always !Sync, but create a platform-specific SyncEventLoopProxy type which is like EventLoopProxy but Sync, and which EventLoopProxy can be converted into through a platform specific-API
  • Create a non-default feature flag which makes EventLoopProxy Sync and document that that feature flag can only be enabled on certain platforms
@madsmtm madsmtm added F - question There's no such thing as a stupid one DS - macos S - api Design and usability labels Feb 1, 2024
@madsmtm
Copy link
Member

madsmtm commented Feb 1, 2024

I mean, EventLoopProxy is Clone, so you can always just clone it just before sharing it with another thread.

That said, the underlying CFRunLoopSourceRef is considered being marked Send + Sync in servo/core-foundation-rs#649, and I'm pretty sure that's correct, the CFRunLoopSourceRef is immutable, so should be safe for us to just add an unsafe impl<T: Send> impl Sync for EventLoopProxy {}`, at least on macOS.

@madsmtm
Copy link
Member

madsmtm commented Feb 1, 2024

Seems like this will need some work on the web too, @daxpedda?

@gretchenfrage
Copy link
Author

Yeah, it is pretty easy to fix a program that fails to compile on macOS for this reason on the user side--I just like the idea that if a winit program compiles on a supported platform and doesn't noticeably use any platform-specific features it should compile on all supported platforms.

By the way, I really appreciate the keyboard input handling rework you all recently pushed through. The fact that it associates the physical/logical and textual input interpretations of a single key press to a single event helped me easily solve a bug of mine that I had previously been using a super hacky work-around for (relating to pressing the T button to open a text input, and not wanting that same T button press to also cause a T character to be typed in after opening it).

kchibisov pushed a commit that referenced this issue Feb 19, 2024
Co-authored-by: daxpedda <[email protected]>
Closes: #3448
kchibisov pushed a commit that referenced this issue Feb 19, 2024
Co-authored-by: daxpedda <[email protected]>
Closes: #3448
jgcodes2020 pushed a commit to jgcodes2020/winit that referenced this issue Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DS - ios DS - macos DS - web F - question There's no such thing as a stupid one S - api Design and usability
Development

Successfully merging a pull request may close this issue.

2 participants