CriticalSection crashes with VS 2022

[ans]

After updating from VS 2019 to VS 2022 and exporting the project with Projucer, I get this crash after starting the app.

It happens when the first singleton is instantiated. It’s the singleton’s lock that causes the crash. Larger context is during dynamic initialization of global variables very early in the startup. The singleton class inherits from DeletedAtShutdown.

What’s happening here? I assume many others have successfully used VS 2022 already, so something weird must begoing on here. What can I look for?

[reuk]

This sounds a little bit similar to the issue described in the linked thread, although that seemed to affect std::mutex rather than the Win32 mutex:

Windows crash in APVTS constructor

@adamwilson looking at this a little more I found the following in the Microsoft STL release notes for VS 2022 17.10 Fixed mutex’s constructor to be constexpr. #3824 #4000 #4339 Note: Programs that aren’t following the documented restrictions on binary compatibility may encounter null dereferences in mutex machinery. You must follow this rule: When you mix binaries built by different supported versions of the toolset, the Redistributable version must be at least as new as the latest tool…

Regardless, if you have any library dependencies, it might be worth checking that you’re statically linking the most recent C++ runtime, or have the most recent dynamic runtime installed and available.

There’s some useful information about toolset compatibility on this page.

[ans]

Thanks. The proposed workaround and solution didn’t help though.

The issue is probably that CriticalSection is not yet ready for use when C++ is initializing global application variables like, for example of this kind:

static inline const MyType namedGlobal = MySingleton::getInstance()->createSomething();

A minimal standalone app project that does nothing else should prove that. I will check that.

[ans]

Yep. That’s it. This example reproduces the crash.

#include "JuceHeader.h"

class MySingleton
{
public:
    JUCE_DECLARE_SINGLETON (MySingleton, false)

    juce::String makeString() { return "Gotcha"; }    
};

static inline const juce::String namedGlobal = MySingleton::getInstance()->makeString();

Too bad. This wasn’t an issue with VS 2019 as far as I remember. I’m using lots of these variables e.g. for icons and other resources, in order to make sure misspellings are caught at compile time. If you look them up in a hash map by name you never learn about typos.

Not sure how to work around this. Maybe singletons could operate lock-free during initialization of global variables?

[ans]

Using JUCE_DECLARE_SINGLETON_SINGLETHREADED_MINIMAL looks like a workaround. But not for fonts. The Juce TypefaceCache is a regular singleton that crashes during startup.

Mutexes are not necessary while an app is initializing variables on a single thread. Still, if classes are involved that happen to use one that should not crash.

Would it make sense to file a bug with Microsoft?

[reuk]

@ans

Would it make sense to file a bug with Microsoft?

Perhaps, if you have a minimal example (i.e. win32 only, no JUCE) that shows EnterCriticalSection crashing during static initialisation.

I’d be interested to see the full stack trace at the point of the crash to rule out other possibilities.

[reuk]

Looking at the example you posted, I think the problem is due to initialisation order.

• JUCE_DECLARE_SINGLETON expands out to (amongst other things) a static data member declaration like:

  static SingletonHolder<blah, blah> singletonHolder;
  

• JUCE_IMPLEMENT_SINGLETON just adds a definition for this static data member in a translation unit:

  decltype (MySingleton::singletonHolder) MySingleton::singletonHolder;
  

• namedGlobal is initialisated at static initialisation time, as is the singletonHolder. If these statics are in different translation units, then their initialisation order is unspecified, so namedGlobal may be initialised before the singleton holder for MySingleton, leading to a crash.

To mitigate, you could:

• init namedGlobal lazily:

  const auto& getNamedGlobal() {
      static const auto result = MySingleton::getInstance()->makeString();
      return result;
  }
  

• or, ensure the correct initialisation order by declaring dependent statics in the same translation unit:

  // .h a
  extern const String namedGlobal;
  
  // .h b
  class MySingleton { /* etc */ };
  
  // .cpp
  JUCE_IMPLEMENT_SINGLETON (MySingleton)
  // only access MySingleton after its holder has been initialised
  const String namedGlobal = MySingleton::getInstance()->makeString();
  

[ans]

Thanks for taking the time to look into this. Here’s the stack trace.

Your proposed solution works. Unfortunately, maintaining a long list with dozens of variables across two files (header and cpp) is something to avoid. I’d rather have the entire list in the header only.

Not sure how to accomplish that yet. Might have to run my own header-only singleton wrapper. At least the cause is clear now.

[ans]

Here’s a header-only simple singleton solution that works so far. The boilerplate can be wrapped in a macro. It has the advantage of not being dependent on initialization order, i.e. genuine “lazy” evaluation.

Static variables with local scope are extremely handy as an alternative to global variables. They spare you the separate “implementation” part.

#include "JuceHeader.h"
using namespace juce;

class MySingleton : DeletedAtShutdown
{
public:                

    static MySingleton* getInstance()
    {
        const ScopedLock sl (getLock());

        if (singletonHolder == nullptr)
            singletonHolder.reset (new MySingleton);
        return singletonHolder.get();
    }

    static void clearSingletonInstance()
    {
        singletonHolder.release();
    }

    ~MySingleton()
    {
        clearSingletonInstance();
    }

    String makeString() { return "Gotcha"; }  

private:
    static inline std::unique_ptr<MySingleton> singletonHolder = {};

    static CriticalSection& getLock()
    {
        static CriticalSection lock;
        return lock;
    }
};

static inline const String namedGlobal = MySingleton::getInstance()->makeString();

[reuk]

Do you need the DeletedAtShutdown behaviour? If not, I wonder why you don’t just use the standard C++11 pattern:

class MySingleton
{
public:
    static MySingleton& getInstance()
    {
        static MySingleton result;
        return result;
    }

private:
    MySingleton() = default;
};

[ans]

For the most part DeletedAtShutdown is needed to avoid a barrage of leaked object warnings that obfuscate/dilute other leak warnings.

[ans]

The current JUCE implementation with the SingletonHolder as a global variable separate from the header causes some stress here. For example, it’s impossible to store an object in a header-defined global variable that makes use of a Font, directly or indirectly. TypefaceCache is just not ready at the time and crashes.

If SingletonHolder was a static inline variable in the header, it would be initialised early and reliably before any user code gets involved. As said, getIInstance() should be callable at any time.

I assume it’s for historical reasons when static inline wasn’t available pre C++17, correct? Any reason for not moving it to the header now that C++17 is minium requirement? It shouldn’t affect any code out there and make things much more robust during startup.

[eyalamir]

@ans

For the most part DeletedAtShutdown is needed to avoid a barrage of leaked object warnings that obfuscate/dilute other leak warnings.

That is true and actually one of the most painful things when dealing with static in JUCE (any struct that might contain, say, juce::String in it, must be contained in a JUCE singleton instead of a regular singleton.

@ans

If SingletonHolder was a static inline variable in the header, it would be initialised early and reliably before any user code gets involved. As said, getIInstance() should be callable at any time.

To my knowledge, if you’re creating a static function that fetches a singleton it would mean the singleton is not shared with other translation units and gets duplicated for each TU.

I think the best way is to use:

1. Header:

MyGlobalObject& getObject();

2. CPP file:

MyGlobalObject& getObject()
{
    return *MyGlobalObject::getInstance();
}

3. Avoid any use of non-singleton globals as the static initialization order in C++ is UB.

I would also suggest to use SharedResourcePointer<MyGlobalObject> whenever possible to further simplify the usage pattern.

[ans]

@eyalamir

To my knowledge, if you’re creating a static function that fetches a singleton it would mean the singleton is not shared with other translation units and gets duplicated for each TU.

Not if you use a static inline variable in class scope (as of C++17). These get linked as a single instance across all compilation units. I’m using that all the time.

[eyalamir]

@ans

Not if you use a static inline variable in class scope (as of C++17). These get linked as a single instance across all compilation units. I’m using that all the time.

I think you’re correct about that.

However - you would probably get the exact same effect - just with the extra safety of lazy creation and if you replace your static members with members that use SharedResourcePointer<T>

[kunz]

I would avoid static variables and singletons whenever possible and keep the state inside classes. There is no need for them.

[ans]

The purpose of a singleton is exactly that. To keep global state encapsulated in the sole instance of a class.

[kunz]

You can also have one class that holds the state without the singleton pattern. Passing a reference to other classes.

Most use singletons because they can read/change the state from everywhere. This has several drawbacks as you already noticed when reporting this bug. It’s also hard to write tests for classes that are using singletons.

But this is just my opinion. In plugins, we shouldn’t use them anyway.

[eyalamir]

@kunz

But this is just my opinion. In plugins, we shouldn’t use them anyway.

Singletons are extremely valuable for plugins for anything that has to be shared across instances.
Examples are all the graphic assets (Images, Fonts, etc), most of which JUCE is already providing singletons for (like ImageCache).

There are more extreme scenarios. For example we make sample-based plugins where the samples have to be held be a Singleton to not eat up the user’s memory.

That’s also true for performance related objects like thread pools.

[kunz]

@eyalamir

Examples are all the graphic assets (Images, Fonts, etc), most of which JUCE is already providing singletons for (like ImageCache).

Yes, this is true. Sharing resources inside a plug-in makes sense, but I don’t want to share these resources between instances. I would prefer a clean state for every new plugin instance and keep things simple.

We now have some popular hosts that start the plugin in dedicated processes. In this case, you may experience different behavior in different DAWs.

For me, sharing things between instances adds complexity I don’t want.

Edit: I have tons of issues because of the static default look and feel class.