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

Allow terminating default TaskPool threads #9477

Open
BigWingBeat opened this issue Aug 17, 2023 · 2 comments
Open

Allow terminating default TaskPool threads #9477

BigWingBeat opened this issue Aug 17, 2023 · 2 comments
Labels
A-Tasks Tools for parallel and async work C-Enhancement A new feature

Comments

@BigWingBeat
Copy link
Contributor

What problem does this solve or what need does it fill?

The way TaskPool is designed means that the only way to terminate its threads is to drop it. Bevy's three default TaskPools are stored in static OnceLocks, which means they cannot be modified or dropped after being initialized. Thus, the threads managed by those pools simply run forever until the process ends.

This is a problem, because I am using Bevy in an environment where all of those threads need to have terminated before the process is able to exit, but currently there is no way to do that. It also means that the on_thread_destroy callback currently never actually gets called.

What solution would you like?

I'm not experienced enough with multithreading and async, nor library API design in general, to know what a good solution for this would look like. Presumably, the way the default pools are stored would need to be changed, to make it actually possible to drop them. An alternative would be to add an API to terminate a TaskPool's threads without dropping it, but I don't think that would be a feasible solution, because of how footgunny it would be to allow people to do that and then continue to use the pool.

What alternative(s) have you considered?

  • Running Bevy single-threaded, by disabling the multi-threaded feature. Probably the easiest solution, but I'd prefer not to have to lose all the benefits of running multi-threaded.
  • Attempting to fix the issue of needing threads to have terminated before stopping the process. I haven't been able to get this to work.
  • Forking bevy to add the aforementioned terminate-threads-without-dropping API, and being careful to only call it when the process is about to exit. This does work, but it would be better if a more proper solution could be found, also so that I don't have to deal with the overhead of using a fork.

Additional context

The environment I'm using Bevy in that is causing these issues, is a C# application running on Mono. I'm using C#'s p/invoke functionality to call unmanaged (Rust) code from C#, and visa-versa. When the application is exiting, Mono attempts to terminate any remaining threads, but freezes up if any of those threads are stuck running native code, as happens with Bevy's default TaskPools.

  • I found this forum thread which seems to describe my problem. The proposed solutions are to clean up all of your native threads before exiting, or to mess with the Windows security attributes stuff. From my testing the former works, but the latter doesn't.
@BigWingBeat BigWingBeat added C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels Aug 17, 2023
@alice-i-cecile alice-i-cecile added A-Tasks Tools for parallel and async work and removed S-Needs-Triage This issue needs to be labelled labels Aug 17, 2023
@BigWingBeat
Copy link
Contributor Author

By the way, the default task pools are currently exposed through wrapper types, that only have get-or-init and panicking getter methods. It would be nice if these wrapper types also had a standard fallible getter that returns an Option or Result.

@BigWingBeat
Copy link
Contributor Author

Thinking about this on-and-off while doing other things, the only reasonable solution I can think of would be to replace the OnceLock<TaskPool>s with RwLock<Option<TaskPool>>s, and expose an Option::take-like method.

If noone else is going to engage with this issue, I'm just going to open a PR that implements this and the other API improvements I want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Tasks Tools for parallel and async work C-Enhancement A new feature
Projects
None yet
Development

No branches or pull requests

2 participants