Android: shouldn't call `std::process:exit` in `EventLoop::run` #2709

rib · 2023-03-01T17:47:05Z

Winit shouldn't be assuming that it's safe to simply terminate the current process via std::process:exit after the event loop returns.

On Android it's possible to run multiple activities in a single process and it's also possible that an application is based on some Java/Kotlin subclass of an Activity that may be expecting various lifecycle events that could be short circuited when Winit simply calls std::process:exit.

Instead there should probably be some logic that leads to calling ANativeActivity_finish (i.e. something to indirectly call Activity::finish() in the JVM) and then entering an infinite AndroidApp::poll_events() loop instead.

The text was updated successfully, but these errors were encountered:

kchibisov · 2023-03-01T18:27:35Z

But that's the reason you can use run_return? The original issue is that some platforms don't allow creating event loop multiple times. Well, that's just ios at this point...

I really don't like that we do -> ! in winit, but it's more of a soundness thing from what I remember. Though, I wonder if we can simply fail in functions, which could be called only once during lifetime.

rib · 2023-03-01T18:45:16Z

Yep, I realize it's possible to use run_return to avoid this if you know how run is implemented on Android but if you don't know about the platform-specific details it's reasonable for an application to use run, but it's not really reasonable that the backend should avoid returning via std::process:exit.

Exiting that way will short normal circuit lifecycle handling - not allowing the Activity to stop and call onDestroy and it also assumes that there is no other Activity running in another thread in the same process.

I also don't really like that run is -> ! but I think handling this on Android can be done a bit more gracefully than by calling std::process:exit. Instead the backend can call the Activity::finish() method in the JVM which should lead to that activity being destroyed - which may even lead to the JVM deciding to kill the process but at least that will be done with consideration of other activities.

After calling Activity::finish() the thread can enter an AndroidApp::poll_events() loop and wait for the activity to be destroyed.

If the JVM doesn't kill the process when destroying the activity I'm not sure there's much that can be done at that point except to stay in a AndroidApp::poll_events() busy loop because it's still not reasonable to call std::process:exit but there also isn't a way to kill the current thread in Rust afik.

I wonder if it would be better if run() could actually return on all platforms, and just make it a platform specific detail that it won't return on iOS.

kchibisov · 2023-03-01T18:52:25Z

I wonder if it would be better if run() could actually return on all platforms, and just make it a platform specific detail that it won't return on iOS.

That will break type system, what I think we should do, is to simply disallow calling run again on ios by returning Error, same as event loop creation. Yes, it's a bit more annoying, but the huge advantage would be for things drop naturally, since exit breaks all destructors/unwinding and such.

In general, I think it'll be wise to return some meaningful results then it's right now.

rib · 2023-03-01T21:34:17Z

Not quite sure what you're referring to with breaking the type system but generally agree with being able return a meaningful Result - and disallowing repeat calls on iOS either via an Error sounds perfectly reasonable to me if it can't be supported.

(In case it was misleading, I wasn't meaning to suggest that on iOS it would still return -> !, I was assuming it would consistently return some kind of Result and just meant that if the implementation for iOS uses some platform-specific API that doesn't return then there would be some iOS specific documentation that clarifies that in practice it will not actually return a Result once the event loop starts)

On Android there is meaningful cleanup we want to do when an application's android_main() returns and it's not ideal that the recommended way of running a Winit event loop makes it impossible to ever return from android_main().

madsmtm · 2023-03-02T10:04:16Z

Another reason we do -> ! is that on web, we have to leak the closure, as well as every static variable created before run.

Perhaps it would really make sense to make a #[winit::main] attribute similar to bevy's? And then have main return the closure instead of calling run?

rib · 2023-03-02T12:21:23Z

Even though a function can return a Result doesn't really mean that it has to do that in any finite amount of time. Not returning is valid behavior imho on any platform where it's not possible to return.

Being able to return a value just gives back a bit more control on platforms where a meaningful status can be returned, without really taking anything away for platforms that can't return. It's the more portable / platform-agnostic approach imho.

I tend to think #[main] like macros should just be optional sugar.

In the case of Android the android_main() gets passed an AndroidApp for context, and there could be more than one android_main run in a single process. android_main also doesn't return a Result. This is awkward to abstract with a platform independent main macro.

Once you introduce a main macro you also probably have to deal with logging initialization which can vary between platforms and it will start to become opinionated and take away some control from developers. (Which can be ok for convenience but other times it's not really what you want)

I think the situation is a bit different with Bevy because it's a higher level framework that can be more opinionated, e.g. with choosing how logging should be handled. Bevy also doesn't really support Android yet (it just has some bare minimum support atm) so it may not be a good reference for being portable yet.

Overall this re-works the APIs for how an `EventLoop` is run to cover these use-cases, with varying portability caveats: 1. A portable `run()` API that consumes the `EventLoop` and runs the loop on the calling thread until the app exits. This can be supported across _all_ platforms and compared to the previous `run() -> !` API is now able to return a `Result` status on all platforms except iOS and Web. Fixes: rust-windowing#2709 2. A less portable `run_ondmand()` API that covers the use case in rust-windowing#2431 where applications need to be able to re-run a Winit application multiple times against a persistent `EventLoop`. This doesn't allow `Window` state to carry across separate runs of the loop, but does allow orthogonal re-instantiations of a gui application. Available On: Windows, Linux, MacOS Could be supported on Android if there's a use case Incompatible with iOS, Web Fixes: rust-windowing#2431 3. A less portable (and on MacOS, likely less-optimal) `pump_events()` API that covers the use case in rust-windowing#2706 and allows a Winit event loop to be embedded within an external `loop {}`. Applications call `pump_events()` once per iteration of their own external loop to dispatch all pending Winit events, without blocking the external loop. Available On: Windows, Linux, MacOS, Android Incompatible With: iOS, Web Fixes: rust-windowing#2706 Each method of running the loop will behave consistently in terms of how `NewEvents(Init)`, `Resumed` and `LoopDestroyed` events are dispatched (so portable application code wouldn't necessarily need to have any awareness of which method of running the loop was being used) In particular by moving away from `run() -> !` we can stop calling `std::process::exit()` internally as a means to kill the process without returning which means it's possible to return an exit status and applications can return from their `main()` function normally. This also fixes Android support where an Activity runs in a thread but we can't assume to have full ownership of the process (other services could be running in separate threads). `run_return` has been removed, and the overlapping use cases that `run_return` previously partially aimed to support have been split across `run_ondemand` and `pump_events`. Fixes: rust-windowing#2709 Fixes: rust-windowing#2706 Fixes: rust-windowing#2431 TODO: Linux + MacOS support

Overall this re-works the APIs for how an `EventLoop` is run to cover these use-cases, with varying portability caveats: 1. A portable `run()` API that consumes the `EventLoop` and runs the loop on the calling thread until the app exits. This can be supported across _all_ platforms and compared to the previous `run() -> !` API is now able to return a `Result` status on all platforms except iOS and Web. Fixes: rust-windowing#2709 2. A less portable `run_ondmand()` API that covers the use case in rust-windowing#2431 where applications need to be able to re-run a Winit application multiple times against a persistent `EventLoop`. This doesn't allow `Window` state to carry across separate runs of the loop, but does allow orthogonal re-instantiations of a gui application. Available On: Windows, Linux, MacOS Could be supported on Android if there's a use case Incompatible with iOS, Web Fixes: rust-windowing#2431 3. A less portable (and on MacOS, likely less-optimal) `pump_events()` API that covers the use case in rust-windowing#2706 and allows a Winit event loop to be embedded within an external `loop {}`. Applications call `pump_events()` once per iteration of their own external loop to dispatch all pending Winit events, without blocking the external loop. Available On: Windows, Linux, MacOS, Android Incompatible With: iOS, Web Fixes: rust-windowing#2706 Each method of running the loop will behave consistently in terms of how `NewEvents(Init)`, `Resumed` and `LoopDestroyed` events are dispatched (so portable application code wouldn't necessarily need to have any awareness of which method of running the loop was being used) In particular by moving away from `run() -> !` we stop calling `std::process::exit()` internally as a means to kill the process without returning which means it's possible to return an exit status and applications can return from their `main()` function normally. This also fixes Android support where an Activity runs in a thread but we can't assume to have full ownership of the process (other services could be running in separate threads). `run_return` has been removed, and the overlapping use cases that `run_return` previously partially aimed to support have been split across `run_ondemand` and `pump_events`. To help test the changes this adds: examples/window_ondemand examples/window_pump_events examples/window_pump_events_rfd The last _rfd example, tests the interaction between the `rfd` crate and using `pump_events` to embed Winit within an external event loop. Additionally all examples have generally been updated so that `main()` returns a `Result` from `run()` Fixes: rust-windowing#2709 Fixes: rust-windowing#2706 Fixes: rust-windowing#2431 Fixes: rust-windowing#2752 TODO: Linux TODO: split patch up

Overall this re-works the APIs for how an `EventLoop` is run to cover these use-cases, with varying portability caveats: 1. A portable `run()` API that consumes the `EventLoop` and runs the loop on the calling thread until the app exits. This can be supported across _all_ platforms and compared to the previous `run() -> !` API is now able to return a `Result` status on all platforms except iOS and Web. Fixes: rust-windowing#2709 2. A less portable `run_ondmand()` API that covers the use case in rust-windowing#2431 where applications need to be able to re-run a Winit application multiple times against a persistent `EventLoop`. This doesn't allow `Window` state to carry across separate runs of the loop, but does allow orthogonal re-instantiations of a gui application. Available On: Windows, Linux, MacOS Could be supported on Android if there's a use case Incompatible with iOS, Web Fixes: rust-windowing#2431 3. A less portable (and on MacOS, likely less-optimal) `pump_events()` API that covers the use case in rust-windowing#2706 and allows a Winit event loop to be embedded within an external `loop {}`. Applications call `pump_events()` once per iteration of their own external loop to dispatch all pending Winit events, without blocking the external loop. Available On: Windows, Linux, MacOS, Android Incompatible With: iOS, Web Fixes: rust-windowing#2706 Each method of running the loop will behave consistently in terms of how `NewEvents(Init)`, `Resumed` and `LoopDestroyed` events are dispatched (so portable application code wouldn't necessarily need to have any awareness of which method of running the loop was being used) In particular by moving away from `run() -> !` we stop calling `std::process::exit()` internally as a means to kill the process without returning which means it's possible to return an exit status and applications can return from their `main()` function normally. This also fixes Android support where an Activity runs in a thread but we can't assume to have full ownership of the process (other services could be running in separate threads). `run_return` has been removed, and the overlapping use cases that `run_return` previously partially aimed to support have been split across `run_ondemand` and `pump_events`. To help test the changes this adds: examples/window_ondemand examples/window_pump_events examples/window_pump_events_rfd The last _rfd example, tests the interaction between the `rfd` crate and using `pump_events` to embed Winit within an external event loop. Additionally all examples have generally been updated so that `main()` returns a `Result` from `run()` Fixes: rust-windowing#2709 Fixes: rust-windowing#2706 Fixes: rust-windowing#2431 Fixes: rust-windowing#2752 TODO: tidy up macos changes - probably lots of debug stuff left in atm TODO: Linux TODO: ask someone to test orbital changes, made blindly TODO: split patch up

Overall this re-works the APIs for how an `EventLoop` is run to cover these use-cases, with varying portability caveats: 1. A portable `run()` API that consumes the `EventLoop` and runs the loop on the calling thread until the app exits. This can be supported across _all_ platforms and compared to the previous `run() -> !` API is now able to return a `Result` status on all platforms except iOS and Web. Fixes: rust-windowing#2709 2. A less portable `run_ondmand()` API that covers the use case in rust-windowing#2431 where applications need to be able to re-run a Winit application multiple times against a persistent `EventLoop`. This doesn't allow `Window` state to carry across separate runs of the loop, but does allow orthogonal re-instantiations of a gui application. Available On: Windows, Linux, MacOS Could be supported on Android if there's a use case Incompatible with iOS, Web Fixes: rust-windowing#2431 3. A less portable (and on MacOS, likely less-optimal) `pump_events()` API that covers the use case in rust-windowing#2706 and allows a Winit event loop to be embedded within an external `loop {}`. Applications call `pump_events()` once per iteration of their own external loop to dispatch all pending Winit events, without blocking the external loop. Available On: Windows, Linux, MacOS, Android Incompatible With: iOS, Web Fixes: rust-windowing#2706 Each method of running the loop will behave consistently in terms of how `NewEvents(Init)`, `Resumed` and `LoopDestroyed` events are dispatched (so portable application code wouldn't necessarily need to have any awareness of which method of running the loop was being used) In particular by moving away from `run() -> !` we stop calling `std::process::exit()` internally as a means to kill the process without returning which means it's possible to return an exit status and applications can return from their `main()` function normally. This also fixes Android support where an Activity runs in a thread but we can't assume to have full ownership of the process (other services could be running in separate threads). `run_return` has been removed, and the overlapping use cases that `run_return` previously partially aimed to support have been split across `run_ondemand` and `pump_events`. To help test the changes this adds: examples/window_ondemand examples/window_pump_events examples/window_pump_events_rfd The last _rfd example, tests the interaction between the `rfd` crate and using `pump_events` to embed Winit within an external event loop. Additionally all examples have generally been updated so that `main()` returns a `Result` from `run()` Fixes: rust-windowing#2709 Fixes: rust-windowing#2706 Fixes: rust-windowing#2431 Fixes: rust-windowing#2752 TODO: X11/Wayland backends TODO: ask someone to test orbital changes, made blindly TODO: split patch up Second pass over the windows backend I had totally underestimated what was involved in updating the windows backend since I hadn't seen the special `wait_thread` that was also dispatching messages and implementing control_flow timeouts via MsgWaitForMultipleObjects, nor had I grokked all the special stuff being done with `WM_PAINT` messages. This "breaks" the MainEventsCleared -> RedrawRequested -> RedrawEventsCleared ordering guarantees, similar to how this is anyway inherently broken in the MacOS backend. The switches to using SetTimer to handle control_flow Wait timeouts and removes the need to use MsgWaitForMultipleObjects. Overall this is a pretty nice simplification of several things TODO: tidy up (lots of debug and misc changes + comments atm) stash

Overall this re-works the APIs for how an `EventLoop` is run to cover these use-cases, with varying portability caveats: 1. A portable `run()` API that consumes the `EventLoop` and runs the loop on the calling thread until the app exits. This can be supported across _all_ platforms and compared to the previous `run() -> !` API is now able to return a `Result` status on all platforms except iOS and Web. Fixes: rust-windowing#2709 2. A less portable `run_ondmand()` API that covers the use case in rust-windowing#2431 where applications need to be able to re-run a Winit application multiple times against a persistent `EventLoop`. This doesn't allow `Window` state to carry across separate runs of the loop, but does allow orthogonal re-instantiations of a gui application. Available On: Windows, Linux, MacOS Could be supported on Android if there's a use case Incompatible with iOS, Web Fixes: rust-windowing#2431 3. A less portable (and on MacOS, likely less-optimal) `pump_events()` API that covers the use case in rust-windowing#2706 and allows a Winit event loop to be embedded within an external `loop {}`. Applications call `pump_events()` once per iteration of their own external loop to dispatch all pending Winit events, without blocking the external loop. Available On: Windows, Linux, MacOS, Android Incompatible With: iOS, Web Fixes: rust-windowing#2706 Each method of running the loop will behave consistently in terms of how `NewEvents(Init)`, `Resumed` and `LoopDestroyed` events are dispatched (so portable application code wouldn't necessarily need to have any awareness of which method of running the loop was being used) In particular by moving away from `run() -> !` we stop calling `std::process::exit()` internally as a means to kill the process without returning which means it's possible to return an exit status and applications can return from their `main()` function normally. This also fixes Android support where an Activity runs in a thread but we can't assume to have full ownership of the process (other services could be running in separate threads). `run_return` has been removed, and the overlapping use cases that `run_return` previously partially aimed to support have been split across `run_ondemand` and `pump_events`. To help test the changes this adds: examples/window_ondemand examples/window_pump_events examples/window_pump_events_rfd The last _rfd example, tests the interaction between the `rfd` crate and using `pump_events` to embed Winit within an external event loop. Additionally all examples have generally been updated so that `main()` returns a `Result` from `run()` Fixes: rust-windowing#2709 Fixes: rust-windowing#2706 Fixes: rust-windowing#2431 Fixes: rust-windowing#2752 TODO: X11/Wayland backends TODO: ask someone to test orbital changes, made blindly TODO: split patch up

This re-works the portable `run()` API that consumes the `EventLoop` and runs the loop on the calling thread until the app exits. This can be supported across _all_ platforms and compared to the previous `run() -> !` API is now able to return a `Result` status on all platforms except iOS and Web. Fixes: rust-windowing#2709 By moving away from `run() -> !` we stop calling `std::process::exit()` internally as a means to kill the process without returning which means it's possible to return an exit status and applications can return from their `main()` function normally. This also fixes Android support where an Activity runs in a thread but we can't assume to have full ownership of the process (other services could be running in separate threads). Additionally all examples have generally been updated so that `main()` returns a `Result` from `run()` Fixes: rust-windowing#2709

This re-works the portable `run()` API that consumes the `EventLoop` and runs the loop on the calling thread until the app exits. This can be supported across _all_ platforms and compared to the previous `run() -> !` API is now able to return a `Result` status on all platforms except iOS and Web. Fixes: #2709 By moving away from `run() -> !` we stop calling `std::process::exit()` internally as a means to kill the process without returning which means it's possible to return an exit status and applications can return from their `main()` function normally. This also fixes Android support where an Activity runs in a thread but we can't assume to have full ownership of the process (other services could be running in separate threads). Additionally all examples have generally been updated so that `main()` returns a `Result` from `run()` Fixes: #2709

This re-works the portable `run()` API that consumes the `EventLoop` and runs the loop on the calling thread until the app exits. This can be supported across _all_ platforms and compared to the previous `run() -> !` API is now able to return a `Result` status on all platforms except iOS and Web. Fixes: rust-windowing#2709 By moving away from `run() -> !` we stop calling `std::process::exit()` internally as a means to kill the process without returning which means it's possible to return an exit status and applications can return from their `main()` function normally. This also fixes Android support where an Activity runs in a thread but we can't assume to have full ownership of the process (other services could be running in separate threads). Additionally all examples have generally been updated so that `main()` returns a `Result` from `run()` Fixes: rust-windowing#2709

This re-works the portable `run()` API that consumes the `EventLoop` and runs the loop on the calling thread until the app exits. This can be supported across _all_ platforms and compared to the previous `run() -> !` API is now able to return a `Result` status on all platforms except iOS and Web. Fixes: #2709 By moving away from `run() -> !` we stop calling `std::process::exit()` internally as a means to kill the process without returning which means it's possible to return an exit status and applications can return from their `main()` function normally. This also fixes Android support where an Activity runs in a thread but we can't assume to have full ownership of the process (other services could be running in separate threads). Additionally all examples have generally been updated so that `main()` returns a `Result` from `run()` Fixes: #2709

madsmtm added DS - android S - api Design and usability C - needs investigation Issue must be confirmed and researched labels Mar 2, 2023

rib mentioned this issue Mar 6, 2023

Winit run_return and Android does not get Resumed and Suspended #2706

Closed

rib mentioned this issue Apr 12, 2023

Re-work event loop run APIs: adds run -> Result<>, run_ondemand and pump_events #2767

Merged

15 tasks

kchibisov closed this as completed in #2767 Jul 27, 2023

rib mentioned this issue Sep 21, 2023

EventLoop 3.0 Changes #2900

Open

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android: shouldn't call `std::process:exit` in `EventLoop::run` #2709

Android: shouldn't call `std::process:exit` in `EventLoop::run` #2709

rib commented Mar 1, 2023

kchibisov commented Mar 1, 2023

rib commented Mar 1, 2023

kchibisov commented Mar 1, 2023

rib commented Mar 1, 2023

madsmtm commented Mar 2, 2023

rib commented Mar 2, 2023

Android: shouldn't call std::process:exit in EventLoop::run #2709

Android: shouldn't call std::process:exit in EventLoop::run #2709

Comments

rib commented Mar 1, 2023

kchibisov commented Mar 1, 2023

rib commented Mar 1, 2023

kchibisov commented Mar 1, 2023

rib commented Mar 1, 2023

madsmtm commented Mar 2, 2023

rib commented Mar 2, 2023

Android: shouldn't call `std::process:exit` in `EventLoop::run` #2709

Android: shouldn't call `std::process:exit` in `EventLoop::run` #2709