Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make with_parent_window safe #3142

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Make with_parent_window safe #3142

wants to merge 1 commit into from

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Oct 13, 2023

Alternative to #3136. EDIT: We'd still need some more work to make Fullscreen Send + Sync on all platforms.

Builds upon #3126 to remove the unsafe from WindowBuilder::with_parent_window, by taking the lifetime inherent in that, and extending it on the platforms where it is relevant.

TODO:

  • Merge the aforementioned PR.
  • Test on all platforms changed
  • Add entry to CHANGELOG.md

@madsmtm madsmtm changed the base branch from master to notgull/rwh06 October 13, 2023 21:42
@madsmtm madsmtm marked this pull request as draft October 13, 2023 21:43
@madsmtm madsmtm changed the title Rwh send sync Make with_parent_window safe Oct 13, 2023
@daxpedda daxpedda removed their request for review October 13, 2023 21:50
@kchibisov
Copy link
Member

Alternative to #3136.

This is not an alternative since it doesn't achieve what said PR achieves.

However doing something like that is fine.

@notgull notgull force-pushed the notgull/rwh06 branch 2 times, most recently from bd80244 to 2933b0a Compare October 14, 2023 16:12
Base automatically changed from notgull/rwh06 to master October 15, 2023 02:07
@kchibisov
Copy link
Member

After looking into it, I don't think it's a good idea as of now to accept owned handler if winit can only provide Raw handles.

This update should be a part of the winit safe handles work.

@daxpedda
Copy link
Member

That should actually be good to go now, right?

@daxpedda
Copy link
Member

After looking at this a bit more closely, I question how thread safety is achieved here exactly. WindowBuilder is Send, but with_parent() can be called on any thread, but WindowHandle isn't Send. So a WindowHandle that has been passed into with_parent() that would be valid on a thread that is not the main thread, is suddenly being used in the main thread.

@madsmtm
Copy link
Member Author

madsmtm commented Dec 25, 2023

thread safety

I think this works on platforms with a "main-thread only" concept, unsure about platforms where a handle may be bound to a specific thread (Windows?)

My reasoning for main thread only being safe on macOS/iOS:

  • WindowHandle<'_> is !Send + !Sync, because it is bound to the main thread.
  • If we get that in with_parent_window, we hence know we're running on the main thread (because of the yet-to-be-documented contract on WindowHandle<'_>).
  • This we put it into OwnedWindowHandle, which contains a MainThreadBound.
  • So now, even though WindowBuilder is moved, we're guaranteed to only access the parent window on the main thread.

@daxpedda
Copy link
Member

daxpedda commented Dec 25, 2023

If the only valid thread is the main thread, I agree there is no problem, because WindowBuilder only gets consumed in the main thread, which is checked by Winit.

Summarizing remaining problems:

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants