-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Return immediately from run_app on web
#4165
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
base: master
Are you sure you want to change the base?
Conversation
|
I think they worked properly as well? Just we had a hack to hold the exit? In general, the point was that you might have a code that does cleanup after the exit in a cross platform manner, and thus, doing an instant return doesn't make much sense from a cross platform behavior. The iOS model is acceptable, since you close the app anyway after so. In general, I'm leaning towards special backends just not being a part of the regular run facility at all, so the difference is clearly stated for them. |
The hack was "throw an exception and cross fingers that Rust doesn't see it and run destructors". Not really what I'd call "worked properly", rather "terribly UB but worked because WASM doesn't yet support exceptions".
On the web and iOS currently, having cleanup after
My problem is that example code suffers when we do not provide a single API that everyone can reliably use (Android is special in that it cannot be a binary, but the others do not need to suffer from this). An alternative would be to provide |
ca7e746 to
7cbc584
Compare
Yeah, but those targets are rather special, so I don't see an issue with them being treated via their own |
|
An alternative would be to provide a As a few data points, Bevy is already effectively merging My point is that all of these examples would benefit from this PR. |
I do somewhat agree here, the differing semantics are confusing (I've tried to remediate this with better documentation), though I guess my point is that to most users it probably won't matter (either they only support desktop, and then they don't care, or they also support web, in which case they'll have tested their app there and seen that it does indeed work as they expect). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this solution at all, but I think its the best we can do right now.
Thank you for doing this!
Wondering, in the ideal world, how would you like Winit's cross-platform entry point to be? |
So in the future, with the Because of iOS not able to return after the If we can't have a cross-platform API that behaves the same on every platform, my next preference is to just not have a cross-platform API. One argument against that is that users often just ignore the differences like this: #[cfg(not(target_family = "wasm"))]
event_loop.run_app(App::default())?;
#[cfg(target_family = "wasm")]
event_loop.spawn_app(App::default());So we agreed that if we can't find a truly cross-platform API, we will go ahead with this PR's proposal. |
|
To expand on iOS: The only public API that Apple provides is I can think of several workarounds to return to the user's code anyhow upon termination, but all of them have costs that I don't think Winit should pay.
|
7cbc584 to
ece00c2
Compare
ece00c2 to
3a8be5b
Compare
|
We discussed this a few months ago, the resolution back then was:
Has this happened? If not, I'd like to move forwards with this PR.
I think that'd be fine with this PR too? We'd document that without |
3a8be5b to
a9dc09c
Compare
I wasn't unable to catch him yet.
Turns out my hopes for TLDR: AFAICS I'm strongly in favor of going back to the old model. It seems to me the only one that is consistent between all platforms and correct. My understanding is that while it is not ideal to Maybe there could even be a way to do this activity detection at run-time and block until other activities have shut-down, but I really have no idea what I'm talking about. I will ping Marijn again. WDYT? |
|
I forgot to mention that there is a way to get rid of the I think it should also be possible to somehow prevent auto implementation of |
641bb23 to
aef7b33
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated this PR per our discussion last meeting, by adding EventLoopExtRegister::register_app and EventLoopExtNeverReturn::run_app_never_return that, together with the already existing EventLoopExtRunOnDemand::run_app_on_demand, allows the user to choose the exact run semantics that they want.
The default EventLoop::run_app still picks the "most desirable" of these based on the platform.
|
EDIT: apologies! Just saw that you commented, let me read first. |
Short circuiting the Activity lifecycle has a user-visible effect on application switching and bypasses the ability for an Androd application save state on shutdown. It really is incorrect to allow run() to exit the process on Android. Winit can't assume responsibility for the whole process. An android-activity app is given a main thread that is conceptually subordinate to the Java thread and needs to be able to gracefully return from android_main, or worst case needs to panic the main thread so that can be caught by android-activity which can call Activity::finish() on the Java thread. Alternatively, Android should not support a run API that can't return. On the other hand, it should always be valid that a Rust API returning a In the end there's no option that's portable across all platforms. |
|
With incorrect I meant UB here.
Can this be actually caught? I'm also not sure what android activity does report back to the system, because AFAIU if its reported that the process failed the app might just restart.
If we deem these drawbacks too steep, this is a good alternative.
While its true that this isn't wrong as per the Rust rules, I think its terribly misleading. In my opinion, if we can't make Android work with You mentioned that You also talked about the whole thread ownership issue, but I don't know exactly what kind of side-effects this actually has. Again: my understanding is that its not "wrong" in the sense of you can work with it and its not UB. Obviously UB is not the line we should draw here, it should be possible to produce a correctly working app with this setup. Even if some features won't work. I also heard that there are ways to solve this on the backend side, like waiting for the activity to finish in Winit. But this would require work on I really want to emphasize that I really don't know what I'm talking about here and the information I gathered so far is close to hearsay, so I really appreciate every drop of input. Just to count off all options right now:
So 1. is what we currently have and want to move away from. So if 2. isn't going to work, we will still have 3. and 4. which will work for most users. fn main() {
...
event_loop.run_and_never_return();
} |
This avoids using JavaScript exceptions to support `EventLoop::run_app` on the web, which is a huge hack, and doesn't work with the Exception Handling Proposal for WebAssembly: https://github.com/WebAssembly/exception-handling This needs the application handler passed to `run_app` to be `'static`, but that works better on iOS too anyhow (since you can't accidentally forget to pass in state that then wouldn't be dropped when terminating).
To allow users to explicitly choose the run semantics that they want.
aef7b33 to
f8f8184
Compare
Builds upon #4149.
Returning immediately from
EventLoop::run_appon the web avoids using JavaScript exceptions, which is a huge hack that has compatibility issues, and doesn't work with the Exception Handling Proposal for WebAssembly.This needs the application handler passed to
run_appto be'static, but that works better on iOS too anyhow (since you can't accidentally forget to pass in state that then wouldn't be dropped when terminating). This effectively reverts the decision in #3006, CC @kchibisov, do you recall if there was a deep motivation for doing that?Since
spawn_app(added in #2208) is now no longer necessary, I've removed it. This means that all the examples should work properly on web again.changelogmodule if knowledge of this change could be valuable to users