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

Can't have global sound objects #970

Closed
CelticMinstrel opened this issue Sep 21, 2015 · 6 comments
Closed

Can't have global sound objects #970

CelticMinstrel opened this issue Sep 21, 2015 · 6 comments

Comments

@CelticMinstrel
Copy link

Using a global sf::Sound object causes the program to crash on exit with an assertion failure. I realize that using global objects is frowned upon, but it would be nontrivial to rewrite my program to not use them. This happens with straight globals, class statics, function statics, and file statics, meaning there's no good workaround. I believe it started when I upgraded SFML to 2.2, or maybe 2.1 - it hasn't always been an issue.

The following minimal code:

#include <iostream>
#include <SFML/Audio.hpp>

sf::Sound my_sound;

int main() {
    std::cout << "SFML Version: " << SFML_VERSION_MAJOR << '.'
        << SFML_VERSION_MINOR << '.'
        << SFML_VERSION_PATCH << std::endl;
    return 0;
}

when compiled with clang++ test.cpp -framework SFML -framework sfml-audio, produces the following output and terminates with SIGABRT:

SFML Version: 2.3.0
AL lib: (EE) alc_cleanup: 1 device not closed
Assertion failed: (lockret == althrd_success), function LockLists,
    file /Users/ceylo/al/openal-soft-1.16.0/Alc/ALc.c, line 779.
Abort trap: 6
@eXpl0it3r
Copy link
Member

I had hoped you'd read my comment, but I guess you didn't...

The destruction order of global objects is undefined. So if OpenAL's globals get destroyed before SFML's global get destroyed you end up with a crash.

As I said and you said yourself, don't use global SFML resources and you'll be fine.

Edit: Don't forget to follow the Contribution Guidelines.

@CelticMinstrel
Copy link
Author

And as I said, it's not easy to change my code to avoid using global SFML resources. (I'm not certain it's actually possible.) However, it's entirely possible to guarantee that an object is not destroyed until after other objects depending on it (it's not even all that hard to be honest), so it's not like this isn't fixable on your end. I also strongly disagree that it's not an issue - a crash is a crash is an issue in my opinion.

I can see that you don't care or want to fix it, but I would be willing to try. Would you at least consider accepting it if I made a pull request for this?

EDIT: By the way, it looks like the code is already set up to try to guarantee correct order of destruction, but it's not working in this case.

@Bromeon
Copy link
Member

Bromeon commented Sep 21, 2015

And as I said, it's not easy to change my code to avoid using global SFML resources. (I'm not certain it's actually possible.)

It's almost always possible to find a design that doesn't use global variables. Of course you have existing code, so refactoring will be required. Using a more structured design is a good idea anyway, it will save you headaches in many other places, too.

I also strongly disagree that it's not an issue - a crash is a crash is an issue in my opinion.

Yes, and we constantly recommend not to use SFML objects, especially not resource-related ones, in global scope. Don't blame the library for deliberately ignoring such advice.

However, it's entirely possible to guarantee that an object is not destroyed until after other objects depending on it (it's not even all that hard to be honest)

No, it's not directly possible, not as long as you maintain ownership and deterministic destruction semantics. In C++, it has always been the default paradigm that objects live during their scope, and dependencies must outlive their clients.

Deviating from that common practice needs considerable book-keeping effort and adds a lot of complexity and overhead to manage dependencies correctly (no, it's not as simple as adding shared_ptr, even if we had it). Even more so, since external libraries are involved. We've had a lot of problems with such hacks in SFML (e.g. OpenGL context or default font), some of which were solved by making things more explicit. I don't like the idea of going the completely opposite direction now and introducing further hacks into the codebase, only to support paradigms that are actively discouraged anyway. I'm not even sure if such changes would be possible without affecting the API.

To get a bit of an idea how "simple" it is to create/destroy things at global scope in defined order, have a look at the longevity pattern and singletons discussed by Alexandrescu. They're explained to their full extent in his book Modern C++ Design, the Loki library is also a good source of inspiration.

@CelticMinstrel
Copy link
Author

I looked into it a bit and it seems the problem is related to OpenAL's use of atexit() to register a handler that cleans up the library, and I can't see a way to ensure SFML has a handler that runs prior to OpenAL's, so I'm not certain it's worth fixing. For my own code, a simple change might be storing pointers to sf::Sound and destroying them explicitly before main() returns... though I don't like having explicit shutdown code like that...

I do think this issue should be left open in case someone thinks of a reasonable way to fix it, but I suppose I can at least prevent the crash now that I know the reason for it.

@Bromeon
Copy link
Member

Bromeon commented Sep 21, 2015

I do think this issue should be left open in case someone thinks of a reasonable way to fix it

I'm afraid that there's probably none. If you look at the longevity pattern, you'll see that it also fiddles with atexit(), but you can only use that to set an order between objects controlled by you, not by an external library. And even if you could, it's still an difficult-to-maintain workaround with pitfalls, for a problem that never appears in clean C++ code.

Those guidelines to avoid global variables in general and for SFML in particular exist for a reason... And in C++, you always have to bear the consequences if you don't follow good practices 😉

@CelticMinstrel
Copy link
Author

For the record, the SFML code in Sound.cpp currently appears to be attempting something vaguely similar to your PhoenixSingleton, but it doesn't work because OpenAL's atexit() is called before SFML's static destruction routines (and adding an atexit() in Sound.cpp doesn't seem to make any difference).

Using pointers like I mentioned above seems to have worked for me, in any case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants