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

Remove unsound Send & Sync impls #2068

Closed
wants to merge 3 commits into from
Closed

Remove unsound Send & Sync impls #2068

wants to merge 3 commits into from

Conversation

jplatte
Copy link
Member

@jplatte jplatte commented Dec 3, 2021

The unsound impls weren't really being relied upon because it was ensured that WindowState was never truly moved or accessed across thread boundaries, but this PR changes things such that they're no longer required, preventing the impls from causing issues in the future.

Fixes #2067.

To still be able to use WindowState in glib closures, use the _local
variants of the idle / timeout callback registration functions, which
will panic if called from a thread other than the one that owns the main
loop.
It is no longer needed since `glib::{idle_add_local, timeout_add_local}`
assert the same thing internally.
@jplatte
Copy link
Member Author

jplatte commented Dec 3, 2021

I made sure that the hello example still starts up successfully on Linux. Should I test anything else to make sure the panic is not hit?

@jplatte
Copy link
Member Author

jplatte commented Dec 3, 2021

Oh, seems like I broke something again. Will look into it in a bit.

@maan2003
Copy link
Collaborator

maan2003 commented Dec 3, 2021

I made sure that the hello example still starts up successfully on Linux. Should I test anything else to make sure the panic is not hit?

try timer, sub window, slow function blocking function and async_event examples

@jplatte
Copy link
Member Author

jplatte commented Dec 3, 2021

Okay, seems like this approach doesn't work because the idea is really that any thread can register a callback with IdleHandle::add_idle_callback, and that callback will end up being called on the UI thread. I think the unsafe impls could potentially be removed by splitting WindowState into a Send part and a non-Send part, I'll try that in the coming days.

@jneem
Copy link
Collaborator

jneem commented Dec 9, 2021

I'm a bit confused by this clippy lint, to be honest. Isn't the whole point of unsafe impl Send to say "I know that some of the fields are !Send, but I promise to use them safely"? Is there any usage of unsafe impl Send that doesn't trigger the lint?

Edit: I found some discussion here. Indeed, it seems like the lint will be demoted until some issues with false positives are resolved.

@jplatte
Copy link
Member Author

jplatte commented Dec 9, 2021

Makes sense. I was also a bit confused, but at the same time I thought that this impl seems rather dangerous. I'll close this for now since I won't have time to try the alternative approach I mentioned above in the near future.

@jplatte jplatte closed this Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gtk: Clippy warns about unsound Send implementation
3 participants