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

Simplify winit runner exit code reporting #13151

Merged
merged 1 commit into from
May 3, 2024

Conversation

Brezak
Copy link
Contributor

@Brezak Brezak commented Apr 30, 2024

Objective

Returning a app exit code from the winit runner is complicated and deadlock prone.
The code to return a app exit code is rather shoddy. It's use of mutex is redundant, It uses unwrap when not required and can be broken by a maintainer simply forgetting to set a value.

Solution

Switch to using a channel.

  • Deals with situations in which a event loop exits unexpectedly.
  • Never panics. Even in extreme cases.

@Brezak
Copy link
Contributor Author

Brezak commented Apr 30, 2024

The original plan was to switch to a arc containing a atomic u8. Although such a solution would use much less memory it would be rather annoying to deal with.

@BD103
Copy link
Member

BD103 commented Apr 30, 2024

Returning an app exit code from the winit runner is complicated and deadlock prone.

Is there a specific issue for this, or do you have a reproduction? I'm curious of the cause behind this change.

@BD103
Copy link
Member

BD103 commented Apr 30, 2024

The original plan was to switch to a arc containing a atomic u8. Although such a solution would use much less memory it would be rather annoying to deal with.

Personally, I think using atomics in this situation would not be too bad; we just use Ordering::SeqCst for everything and follow up by seeing if we can lower it to Aquire and Release.

Then again, I may be more familiar with concurrency.

@Brezak
Copy link
Contributor Author

Brezak commented Apr 30, 2024

Returning an app exit code from the winit runner is complicated and deadlock prone.

Is there a specific issue for this, or do you have a reproduction? I'm curious of the cause behind this change.

My phrasing was rather hyperbolic, sorry. It was predicated on the possibility of the event_loop getting leaked for some reason. I've realized that this wouldn't actually deadlock but would cause a panic when the Arc::get_mut call at the end of winit_runner would fail.

The original plan was to switch to a arc containing a atomic u8. Although such a solution would use much less memory it would be rather annoying to deal with.

Personally, I think using atomics in this situation would not be too bad; we just use Ordering::SeqCst for everything and follow up by seeing if we can lower it to Aquire and Release.

Then again, I may be more familiar with concurrency.

Using a channel means we get to detect situations in which, a future maintainer working on the event loop forgets to set an exit code before exiting the event loop. Though my primary reasoning for not using atomics is that the code would be harder to parse.

On the ordering of operations. We could use Ordering::Relaxed. If nothing's gone wrong (see concerns with panic above) synchronization between sending and receiving a exit code is guaranteed by the event loop itself exiting only after a exit code was stored.

Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with the event loop, so I recommend getting one more approval before merging.

Either way, your reasoning makes sense! I have a few small notes, but nothing blocking.

crates/bevy_winit/src/lib.rs Show resolved Hide resolved
crates/bevy_winit/src/lib.rs Outdated Show resolved Hide resolved
@BD103 BD103 added A-Windowing Platform-agnostic interface layer to run your app in C-Code-Quality A section of code that is hard to understand or change labels May 1, 2024
@alice-i-cecile
Copy link
Member

@pietrosophya could you take a quick look at this if you have the time and interest?

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 3, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 3, 2024
Merged via the queue into bevyengine:main with commit 15687b5 May 3, 2024
28 checks passed
@Brezak Brezak deleted the exit-with-channel branch May 6, 2024 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants