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

Support 'atexit'-based cleanup on Windows #849

Conversation

vittorioromeo
Copy link

@vittorioromeo vittorioromeo commented May 20, 2024

Hello!

We have started using miniaudio in SFML 3.x and so far our experience has been great.

As an attempt to simplify our audio resource management, I attempted to remove run-time based reference counting in lieu of a simpler manager object with a static lifetime -- an important first step towards adding support for multiple audio devices in our library (see SFML/SFML#2974).

My changes unfortunately ended up causing problems on Windows, as any application using our audio module freezes on termination. After some investigation by @binary1248 and some further tests by myself we realized that the problem is due to how Windows handles atexit and thread termination when building as a shared library.

Basically, as part of process termination procedure, the Win32 runtime will forcefully terminate any remaining thread without joining them nor sending a thread-detach notification. This means that anybody trying to cleanup miniaudio resources via atexit (which is exactly how C++'s static object destructors are invoked) will end up with a program hanging on ma_event_wait(&pDevice->stopEvent) inside ma_engine_uninit, because pDevice->thread was terminated and there was never a chance to signal the stop event.

A simple workaround for this issue is to check if the thread is still alive prior to waiting on stopEvent. I've done some research and found this solution that suggests it's sufficient to check WaitForSingleObject(threadHandle, 0) == WAIT_TIMEOUT to see if the thread is still alive.

Applying the changes to miniaudio.h and trying them out under our SFML test suite both as a static and as a shared library seems to resolve the issue and support our use case.

As an added benefit, any C or C++ developer using atexit for miniaudio cleanup will benefit from this change, and any other C++ developer using static lifetime for miniaudio cleanup will also do so.

Thanks!

@vittorioromeo
Copy link
Author

I just realized this PR should also resolve #823 and #634.

@vittorioromeo
Copy link
Author

vittorioromeo commented May 20, 2024

I might have spoken too soon... this solved the issue locally for my on my Windows setup, but still seems to hang in our CI. Needs more investigation.

@mackron
Copy link
Owner

mackron commented May 20, 2024

Thanks a lot for this. This change as it stands looks good to me (just a very minor thing is that the shouldWait variable should be declared at the top of the block because I'm maintaining C89 support - I'll just address that manually). Would you like me to merge this now, or would you rather I wait for your CI investigation?

@vittorioromeo
Copy link
Author

Thanks a lot for this. This change as it stands looks good to me (just a very minor thing is that the shouldWait variable should be declared at the top of the block because I'm maintaining C89 support - I'll just address that manually). Would you like me to merge this now, or would you rather I wait for your CI investigation?

Please wait, this workaround might not actually be consistent unfortunately. I am doing more research, but it seems like there's no foolproof way of actually achieving this without having the user manually invoke a cleanup function at the end of their main. :(

I opened a discussion on StackOverflow if you are interested:
https://stackoverflow.com/questions/78507797

@mackron
Copy link
Owner

mackron commented May 20, 2024

It's an awkward one. An obvious "solution" is to simply not shutdown miniaudio explicitly and just let the OS deal with releasing all the relevant resources. But it just doesn't feel right doing that. And I'm imagining tools like memory leak detectors would complain as well.

The only other thing I can think of on the miniaudio side is to have a ma_device_uninit_atexit() or something which just shuts everything down with the assumption that there aren't any threads running, but that feels exceptionally hacky. Open to ideas!

@mackron
Copy link
Owner

mackron commented May 20, 2024

Would it be possible for miniaudio itself to register it's own atexit callback which simply sets a global variable indicating that application shutdown is happening, and then in ma_device_uninit(), if that variable is set, just skip the parts where it waits for any threads to finish up? I've never used atexit before so not sure if that's a stupid idea or not...

@mackron
Copy link
Owner

mackron commented Aug 3, 2024

I'm going to close this one. The way to do it is to just not clean up and let the operating system deal with releasing the processes memory. It's not clean, but I can't see any other real practical way to address this.

@mackron mackron closed this Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants