Panic on multiple EventLoop instantiation, document why

[MarcusGrass]

• Tested on all platforms changed (Only tested on x11, although this change is platform agnostic)
• Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
• Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
• Created or updated an example program if it would help users understand this functionality
• Updated feature matrix, if new features were added or implemented

Since it was extremely easy to implement using existing dependencies I just made a prototype implementation that agressively fixes #2166.

In the issue discussions were around just documenting it, or making it panic. I'm more for making it panic immediately since the problems can be hard to diagnose if they occur "naturally" such as in the report of a segfault, with other example in that same issue of SendErrors. Getting a segfault and tracking it down to EventLoopBuilder's docs will probably waste a lot of time for users.

I'm not ecstatic about having a static OnceCell to guard against double instantiation, but I can't think of any smoother solution

[maroider]

I don't have much of anything to point out beyond what @kchibisov has already done. I would like a better panic message, but I can't seem to formulate something that's both unambiguously better and doesn't remove the "this isn't supported" part right now.

[MarijnS95]

Just in case anyone comes across this error on Android:

Its framework can launch multiple activities inside the same process. This activity start however pretends to be like a main() call that is typically only called once, nor does ndk-glue properly support >1 activity running at a given time (everything goes bonkers otherwise). If users create their EventLoop inside that fake main() function, they'll run into this error at some point.

That's not something caused by this PR though, rather it's highlighting the underlying issues that we have yet to resolve ;)

[kchibisov]

We could have platform specific methods to avoid this error, like new_any_tread methods available on certain platforms, like Wayland/X11. Something like that could be done for Android.

[MarijnS95]

Please no. This is "yet another ndk-glue misunderstanding" that has to be corrected rather than swept under the rug. Its design doesn't allow for a use-case that's clearly possible.

When does this happen? Depends on how Android launches the users' activity; on my phone it at least relaunches when switching the app (in)to a so-called "pop-up window", allowing me to "test" resizes (and it took a while to understand why it was completely and utterly broken 😬).

[grovesNL]

Should we clear the static on Event::LoopDestroyed? There are some potential use cases where it might make sense to recreate the event loop (e.g. wasm module where the process fully exits but the OnceCell remains set) or have multiple event loops (e.g. when not using run as described in #1714).

[kchibisov]

I think some platforms(macOS/ios) will abort if you try to recreate the event loop, the error is for a particular functions to prevent cross platforms issues, however we can provide separate methods that allow event loop recreation.

[grovesNL]

@kchibisov Platform-specific extensions would be ok. I just thought it might be cleaner to clear the OnceCell in one place if multiple platforms support event loop recreation.

With the current API, is it possible to attempt to recreate the event loop on macOS and iOS without having to go through one of the non-portable paths (e.g. run_return)? If we have to use run_return in order to recreate the event loop on macOS/iOS anyway, maybe we could document the restriction there.

[kchibisov]

run_return isn't about recreating the loop, it's not about loop dropping like you do run_return and then std::mem::drop(event_loop). Run return basically means that you can call run_return, poll events you want into some buffer, exit closure, processes events, render, and call run_return again. It's more about polling events on demand, not dropping the event loop.

EventLoop creation and dropping is complicated topic wrt recreation since system libraries to initialize some global state most of the time. On drop it usually persist or something else has non-null state, etc, etc.

So I'm not sure what needs to be documented, in fact I'm not sure there're platforms safely allowing event loop recreation, most of the time it's never tested and if it works you're lucky.

[grovesNL]

Yeah I agree with that. I mean you can't normally return from run, so in general it's not clear if it would be possible to recreate the event loop after its running without going through a non-portable path (e.g. run_return). I was wondering if there's any downside to clearing the OnceCell on Event::LoopDestroyed for all platforms since the event loop can't usually be recreated when using run.

On the web it can be a bit tricky avoiding recreating the event loop (e.g. single page application that uses winit on a single tab). Even though we're using run, destroy the event loop, and exit the process, the static OnceCell remains set in the wasm module. Normally this works pretty well, but I had to revert this PR in my winit fork as a workaround to avoid the OnceCell already being set. It's probably possible to wait for GC to fully unload the wasm module or work around it in other ways.

[kchibisov]

for all platforms since the event loop can't usually be recreated when using run.

Well, for run it doesn't make any sense, since run terminates the program. If the platform supports multiple EventLoop::new I'd suggest to add a platform specific method on EventLoopBuilder, like build_web, which will avoid checking the variable in the first place.

[grovesNL]

I've been using run in this way on web without any issues - the entry point that calls run terminates, but not the wasm module containing the OnceCell state.

I'm ok with adding platform-specific extensions to handle this use case. I thought it might be cleaner to clear the OnceCell when the event loop is destroyed to avoid leaving it set forever.

[kchibisov]

I think what you want is #1714 and impl is #2208. So picking that PR should work?

[grovesNL]

Right, #1714 / #2208 will definitely make this easier, but we'd still have the same issue if the OnceCell is shared between calls to spawn. In other words, if the OnceCell isn't cleared during Event::LoopDestroyed or when the caller exits, we still won't be able to recreate the event loop.

[kchibisov]

I think we can probably clear it from spawn? I'm fine with it given that web is a bit special here.