sf::Texture crashes when used in a static object

[Tank]

A lot of SFGUI users experience crashes when the program is exitting. This is due to sf::Texture being destroyed after sf::Context is destroyed and happens because a lock is tried to acquired on a mutex that's static and has already been destroyed, too.

binary1248 did a quick example (and also found this one out). You HAVE TO compile it with MSVC, because the crash doesn't happen with GCC (surprise, surprise ).

Here's binary1248's original post, I just append it here: (source: http://sfgui.sfml-dev.de/forum/post316.html#p316)

binary1248 from here on

Code: [Select]

#include <SFML/Graphics.hpp>

class Singleton {
public:
  static void instanciate() {
    instance.reset( new Singleton );
  }
private:
  static std::tr1::shared_ptr<Singleton> instance;

  sf::Texture texture;
};

std::tr1::shared_ptr<Singleton> Singleton::instance;

int main() {
  Singleton::instanciate();

  return 0;
}

Compiles and crashes on Windows 7 VS2008.

To quote a famous TV personality I find entertaining: Well THERE'S your problem.

Explanation:

In Visual C++ static variables are destroyed in a specific order, this being different from GCC obviously. When trying to destroy the sf::Texture, SFML tries to make sure that it has a valid context to destroy it with. This in turn tries to create a new context because the previous one was already destroyed. When creating a context it has to lock a mutex (a.k.a enter a critical section on windows) which is declared static inside createContext. This requires the critical section to be initialized before use and destroyed after done using. Because the Mutex is static the critical section it contains was destroyed immediately at the end of the application and stupid MSVC can't realize it is trying to access a static variable it already destroyed and doesn't want to initialize a new one. EnterCriticalSection then crashes because it is passed an invalid CRITICAL_SECTION object.

There you go. Another case of MSVC and SFML playing nice together.

I'm too lazy to report this somewhere so if anybody could mention this or link Laurent to this post I would be very very grateful.

[Laurent]

I'd say that doing things outside main(), especially things that involve resources & other APIs, is very dangerous and should be avoided.
Don't replicate the same mistakes as SFML, write better code

Seriously, I'd be glad to remove all the global stuff that I use, but unfortunately I haven't found a clean replacement yet.

Another case of MSVC and SFML playing nice together

This behaviour conforms to the standard, as far as I know. The order of destruction of global objects is undefined across translation units, and using a destroyed object is undefined too.

[binary1248]

And what makes it worse is the fact that objects from different translation units and possibly different executable modules rely on each others existence to function properly.

I normally avoid using static variables because they are so... unpredictable... but because I needed a singleton object there was no other way. That being said I guess singletons aren't compatible with the way SFML handles it's GL contexts?

If you ask me, the true problem here is tracking the dependencies of static objects among each other. Because their destruction order is undefined they have to be locked from spontaneous destruction much like how the context is now (waiting for the last resource to be destroyed).

One thing has puzzled me since I figured this out: Why does the context even get destroyed before the sf::Texture does? The whole problem is that a new context is being created after the application has exited anyway just so that the sf::Texture, which was created with the previous context, can be destroyed? It seems a bit strange.

Perhaps it is because faulty mutex locking is also causing the reference counting in GLResource to break? Either way I think the sf::Mutex class has to be made more robust to support static instances better.

EDIT:

After a bit more research I found these useful links:
http://blogs.msdn.com/b/oldnewthing/archive/2004/03/08/85901.aspx
http://www.parashift.com/c++-faq-lite/ctors.html Articles 10.14 to 10.18 are of interest.

Now hopefully anybody reading this realizes that playing with static variables is literally deadly. That said, having a static Mutex in WglContext.cpp is like asking for a fully loaded revolver in Russian roulette.

[Lee R]

[...]but because I needed a singleton object there was no other way.[...]

There is always another way Use the service locator pattern instead.

[binary1248]

[...]but because I needed a singleton object there was no other way.[...]

There is always another way Use the service locator pattern instead.

I'm always up for something new. The question is: will Tank give the green light?

Even more research:

According to the reply in this http://stackoverflow.com/questions/3143180/how-to-do-static-de-initialization-if-the-destructor-has-side-effects-and-the-ob, compilers reinitialize static variables in the reverse of how they were initialized. Logic tells me that because the Mutex in WglContext.cpp is only constructed after the WglContext itself is that that static Mutex is also destroyed before the WglContext is. Upon creation of another WglContext it isn't recreated because it was already created at the start of the application and therefore just ignored?

There must be a subtle difference between how GCC and MSVC handles static variables and there are 2 possible scenarios. Either both don't conform with the standard or one does and the other doesn't. Either way I think GCC is being smarter about static vars than MSVC.

[Laurent]

I normally avoid using static variables because they are so... unpredictable... but because I needed a singleton object there was no other way

Can't you call a cleanup function at the end of the main()? You're not forced to let the instance be destroyed automatically after main(), you can kill it manually and force a predictable and correct behaviour.

One thing has puzzled me since I figured this out: Why does the context even get destroyed before the sf::Texture does?

I have to look at my code again, but I don't think it's the expected behaviour. But it would be the same even if the context was still there: it would have to be destroyed after that, and destroying a context involves the same stuff (mutex) as creating one.

That said, having a static Mutex in WglContext.cpp is like asking for a fully loaded revolver in Russian roulette.

Only if you do dangerous things in your own code

[Tank]

I'm always up for something new. The question is: will Tank give the green light?

Fixes get instant green light.

fully loaded revolver in Russian roulette.

ymmd

To me it seems both sf::Context and sfg::Renderer are show-stoppers here. I know that the automatic construction of both is to ease the usage of both libraries, but if that leads to problems, I think it's really not worth it.

SFML uses a specific class (sf::Window and derivates) that everyone needs to be able to render stuff. Therefore managing the context yourself would be quite easy I guess:

Code: [Select]

int main() {
  sf::Context context;
  sf::RenderWindow( ..., context );
}

That way it's guaranteed that the context dies after everything else with just one more line of code. If you really want dangerous behaviour, you could provide another ctor that doesn't need a context reference and constructs the static context (just like it's done now), but if you asked me it's better that people write that extra line instead of wanting to be as easy as possible.

Of course the example implies that it's even possible to work with an sf::Context reference, I haven't checked SFML's sources.

As for SFGUI I don't have an idea yet; the simplest would be init() and cleanup(), but that's often not needed in C++ and is also annoying.

[Laurent]

SFML uses a specific class (sf::Window and derivates) that everyone needs to be able to render stuff. Therefore managing the context yourself would be quite easy I guess

That wouldn't make things better for people who destroy sf::Textures after the main()
And for people who don't, everything already works fine with the current code.

[Tank]

, that's true.

[Tank]

Alright, did some brainstorming with binary1248 and came up with one possible solution.

Diagram first:


Let's take sf::Texture as an example:
An sf::Texture object is created and you call loadFromImage() on it. sf::Texture then calls sf::Context::create_resource() to create the appropriate texture resource and gets back a proxy object (or even better: turn sf::Context into a factory that creates objects like sf::Texture).

The resource proxy holds a weak pointer to the real resource that's being owned by sf::Context. That means that when the Context is getting destroyed, all resources will be destroyed to and the proxies reference to nullptr (sf::Texture has to check for this case, of course).

If sf::Texture gets destroyed before sf::Context, then ResourceProxy's destructor will unregister the resource at sf::Context.

This can be done simply using shared_ptr/weak_ptr or an own implementation of reference counting and especially weak references to detect when a resource got invalid.

Another option (which should be considered indepedently from the crash bug, too!) is removing the shared context stuff, which would also remove the need to wait for static resources being destroyed. Instead of having a shared contexts, sf::Window should completely own and manage one.

I don't know all the benefits, but also for having multiple windows I don't think it's worth all the trouble.

[Laurent]

All I can say is that it is a very complicated issue. Probably the most complicated one in SFML. There are many things to consider, and solutions that might appear simple are generally not covering all the use cases.

To tell you what's good and what's wrong with your solution, I'd have to dive in my code again, and I'm afraid I won't have the time to do that right now. What I suggest is that you create an issue on the tracker, that we continue to put as many details and ideas there, and I'll get back to it seriously after SFML 2.0 is released.

[Tank]

The issue exists since sf::Font::GetDefaultFont(). Imho it's not a good idea to release a 2.0 when knowing something like that exists.

Implementing resource control is surely something not so trivial and takes some time, but dropping shared contexts could be done quite easily. Yes, it would remove the possibility of creating multiple windows, but seriously, who *really* needs that? Also it won't be possible to use the context in separate threads, but that's something everybody should avoid *always* (it isn't compatible between all platforms and often/always leads to strange behaviour/crashes).

So until you've got more time on hands for SFML, can't you just disable shared contexts for SFML 2.0? It would at least solve a lot of problems in a lot of places (SFGUI & FlexWorld, just to name two bigger projects who ran into that issue quite early).

And while we're at it: Didn't you want to think about sf::Font::getDefaultFont() returning a copy of a new sf::Font instead of a static version?

[Laurent]

How do you load textures in a separate thread if I remove shared contexts?

Imho it's not a good idea to release a 2.0 when knowing something like that exists.

Hopefully this can be fixed without changing the public API, so it doesn't block me from releasing 2.0.

[Laurent]

And multiple windows is an important feature that many people use, I don't want to remove it.

[Tank]

How do you load textures in a separate thread if I remove shared contexts?

You don't. Believe me I ran into so many problems regarding exactly that, that I decided to drop the idea of using a context in another thread than the "primary" thread.

What you can do is loading the image data in a separate thread. But texture creation should still be done in one single thread.

Also think about how the interaction with the driver/GPU works: There's one bus which isn't threaded by any means. That means you don't have a benefit at all when you're preparing your textures in a separate thread.

And multiple windows is an important feature that many people use, I don't want to remove it.

Out of curiosity: Who? I really can't think of a single use-case where multiple windows couldn't be emulated by OpenGL.