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

Fix example return_after_run #1082

Merged
merged 4 commits into from
Dec 18, 2020

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Dec 17, 2020

First crash

Example return_after_run is currently crashing when closing the first window:

     Running `target/debug/examples/return_after_run`
Running first App.
Running another App.
thread 'main' panicked at 'Could not set global default tracing subscriber.: SetGlobalDefaultError { _no_construct: () }', crates/bevy_log/src/lib.rs:83:22
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The issue is that as the default plugins setup a global tracing subscriber, doing it a second time fails. https://docs.rs/tracing/0.1.22/tracing/dispatcher/fn.set_global_default.html for more information

My fix is to ignore errors, as it just means that there is already a subscriber configured.

As an added bonus, the user can now set up their own subscriber however they want.

Second crash

The example was still crashing:

     Running `target/debug/examples/return_after_run`
Running first App.
Running another App.
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', crates/bevy_winit/src/lib.rs:343:66
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

After some investigation, it's because the event loop for the second app receives an event Focused(false) from the window of the first app. It can be reproduced when using only winit, and looking at how it works it may be platform dependant (I'm on macOS) and events are just translated from the OS, so not something fixable.

My fix is to ignore Focused(false) events for unknown window, as they are pretty much meaningless anyway. I kept the crash for a Focused(true) event on an unknown window.

It runs!

Now it works :)

     Running `target/debug/examples/return_after_run`
Running first App.
Running another App.
Done.

@refnil
Copy link
Contributor

refnil commented Dec 18, 2020

I had the same errors while doing integration tests (#1057). I've had not submitted a PR for the subscriber thing yet because I used the workaround of not including the LogPlugin in my test. Still, I checked a bit the documentation of the tracing library to see if there was something that we could to fix the set_global_subscriber problem.

I don't think silently ignoring the subscriber errors is a good idea long term. We could still merge this but open an issue to make it better later. I think a have three options.

  1. Add an option in LogConfig to disable setting the subscriber. It would definitely work and is pretty simple but it feel weird since you the resources can invalidate the LogPlugin completely. Then, could we just prevent adding the LogPlugin then? Well, that is annoying for the user since it cannot use DefaultPlugins. Finally, the resource need to be added before the LogPlugin since the subscriber is set on the plugin build step and not in a startup system.
  2. Put the LogPlugin behind a feature flag. You can then disable the logging if you application encounter the bug. The problem is that you probably still want logging in tests or in your application. Maybe that would mean that you would set the default subscriber yourself before building and running your apps.
  3. Have a tighter integration between BevyLog and BevyTask (if I remember correctly). The idea is to set the created subscriber as a default for each thread used by bevy. BevyLog can also set it for the main thread. Then, when thread a created in BevyTask, you need to get the dispatcher on the current thread and set it on each new thread that is created. This is my favorite solution. I found that tokio was doing something like this but they remove all logging last year. It is the only solution that would allow different bevy app to have different logging config/setup. However, I'm not sure about the performance. Is it better? Is it worse? Also, logging wouldn't work on user created thread.

I hope that some of that make sense.

Next, let talk about the window id issues. I see that you fixed it only for the Focused event. My question would be: what does it mean to receive an event for another window? Should we do anything? I don't think so. That why my PR (#1072) sanitize event for all window event. The downside is a slight degradation of performance on events that would get the window object. I also work on the assumption that all event could be received for the wrong id. I may be wrong about that. If we are convinced that only the Focused event could be received with the wrong id, then you solution has merit. (Maybe I would recommend add my settings to let the user choose between panic and ignoring).

@Moxinilian Moxinilian added A-Core Common functionality for all bevy apps P-Crash A sudden unexpected crash A-Windowing Platform-agnostic interface layer to run your app in labels Dec 18, 2020
@mockersf
Copy link
Member Author

You're right about logs, I changed the PR:

  • improved the error message in case of error telling what to do
  • example now disable the LogPlugin for the second run, and shows that log is still working

@refnil
Copy link
Contributor

refnil commented Dec 18, 2020

That looks good to me for the subscriber thing. I didn't know about add_group_with.
As for the window id problem, I'll let other comment on it since I'm partial to my own fix.

@cart
Copy link
Member

cart commented Dec 18, 2020

This looks good to me. @refnil I prefer "one way that works by default" instead of giving people too many choices. I'm going to merge this because 0.4 is dropping soon. But I do like the approach in your PR to factor out the winit window fetch. It cuts down on duplicate boilerplate and ensures we ignore events the same way everywhere. If you can rebase your changes on master (after I merge this pr) and then remove the configuration option, I'd love to merge those changes (although i won't block the 0.4 release on it).

@cart cart merged commit d0840bd into bevyengine:master Dec 18, 2020
@fopsdev fopsdev mentioned this pull request Jan 24, 2021
@mockersf mockersf deleted the fix-example-return-after-run branch April 27, 2021 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps A-Windowing Platform-agnostic interface layer to run your app in P-Crash A sudden unexpected crash
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants