Skip to content

Exit schedule for systems to run at end of app run #7355

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

Closed
wants to merge 1 commit into from

Conversation

Testare
Copy link
Contributor

@Testare Testare commented Jan 24, 2023

Objective

Adding a structure for systems that run only when the App is closing.

Fixes #7067

Solution

Adds a Schedule to the default stages with label ExitSchedule to run systems at the end of the App.

Changelog

Added

  • Created ExitSchedule struct as label for new exit schedule.
  • Created ExitStage struct as label for new exit schedule's only stage, since it seems schedules need at least one stage currently. I presume this will be easy enough to remove after the stageless RFC.
  • Updated AppExit documentation to mention exit systems.
  • Added add_exit_system and add_exit_system_set APIs to App to add systems to the exit schedule.
  • Added teardown API to App so that runners can call the exit schedule.
  • Test for default schedule runner to verify exit systems are run when the App is closing.

Changed

  • Default schedule runner now calls App::teardown before exiting.
  • Winit schedule runner now calls App::teardown before exiting.
  • Added ExitSchedule to the App::add_default_stages() method.

Migration Guide

  • Custom runners would have to call App::teardown in order to run exit systems.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events S-Blocked This cannot move forward until something else changes X-Controversial There is active debate or serious implications around merging this PR C-Feature A new feature, making something new possible labels Jan 24, 2023
@alice-i-cecile
Copy link
Member

I really want this, but you're going to need to wait until #7267 is merged, and then use the Schedule machinery from there, likely by modifying the default CoreSchedule::Outer behavior.

@mockersf
Copy link
Member

I really don't want this, or you need to put heavy disclaimers that there are no guarantees any code in this schedule will run

@Testare
Copy link
Contributor Author

Testare commented Jan 24, 2023

Didn't know you could see draft pull requests ^-^' This is my first PR on a scale more than a small bugfix.

Yeah, I realized pretty early into implementation that this would touch stageless stuff, and tried to write it in a way I figured would be easy to rework without stages, and didn't add more than a single stage to the ExitSchedule. But it sounds like the schedule changes are deeper than I thought, so I'm fine waiting until stageless is merged.

As far as whether this should exist or not, I feel like that's a conversation above my paygrade here. I tried my best to be transparent in the documentation, and I'm fine adding a disclaimer about there being no guarantees for exit systems to run. For the documention, situations that would probably not run exit systems would be:

  • User force-quits the program
  • panic!
  • custom App.runner does not call teardown() before exiting.
  • others?

@mockersf
Copy link
Member

Didn't know you could see draft pull requests ^-^' This is my first PR on a scale more than a small bugfix.

No worries!

As far as whether this should exist or not, I feel like that's a conversation above my paygrade here.

There aren't that much paygrades around, and your opinion matters and is valued.

For the documention, situations that would probably not run exit systems would be:

  • User force-quits the program
  • panic!
  • custom App.runner does not call teardown() before exiting.
  • others?

That comes to mind:

  • Running in wasm, iOS or Android
  • User shut down their computer or any other event of this kind

@Testare
Copy link
Contributor Author

Testare commented Jan 24, 2023

Awwww, warm fuzzies! Merci mon pote!

Okay, that makes sense. Of course if the user cuts power no exit schedule would run.

The iOs/Android comment brings an idea to mind. From the one time I tried mobile development, I vaguely remember that there were hooks that give some time to run code before an app is closed. Is there a way to take advantage of these hooks in current bevy?

Here's my summary of general pros and cons of this:

Pros:

  • Have a dependable place for systems only meant to run at end of program: Does not depend on the system being scheduled after all systems that writes AppExit.
  • Seems like a natural complement to startup systems.
  • Potential for taking advantage in the future of target-specific exit handling in a way that regular systems can't, such as running exit systems in response to an iOS "applicationWillTerminate" hook. (Some of these, such as the Web API "beforeunload" hook, are still a little undependable)

Cons

  • There are still many situations where exit systems will still not execute,
  • Not a lot of immediate need: For situations like system resources Drop can handle it better.
  • Having an official exit system API might communicate something more dependable, and if it works better on some targets than others it can cause problems if used naively in plugins meant to be universal.

Perhaps a good way to get the best of both sides would be to create the specific exit schedule, add handling for it in runners, but we don't add an API specifically for exit systems to main impl App block.

We can instead create a trait WithExitSystems that includes the add_exit_system<_set>() methods. We implement this for App, but we don't include this trait in the bevy prelude. This prevents the functionality from being used naively via intelli-sense or anything like that, but if people want the functionality they can find it in the docs with all the caveats attached. But if they read and understand the issues, to use it would only require them adding the trait to their imports.

Those are my ideas on this anyways!

@alice-i-cecile
Copy link
Member

We should disable this feature on the platforms this doesn't work on. Probably wrap it all in a feature flag.

@stephenmartindale
Copy link
Contributor

We should disable this feature on the platforms this doesn't work on. Probably wrap it all in a feature flag.

  1. The Cargo Book specifically states: "... it should usually be safe to enable any combination of features."

    • I.e. having the feature available at all – even disabled on a platform – exposes crate consumers to the choice to enable it. If enabling it adds functionality that doesn't work on that platform, you're violating the intended use of the features feature.
    • Features can be – and frequently are – enabled transitively and, once enabled, there is no way to negate them.
  2. rust-lang/cargo № 1197 is still open after EIGHT YEARS.

@Testare
Copy link
Contributor Author

Testare commented Nov 8, 2023

Closing this as the cons outweigh the pros and there hasn't been much interest in this for almost a year

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible S-Blocked This cannot move forward until something else changes X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Teardown/exit systems
4 participants