Can I access a static local while it is being constructed in C++?

Question

Static locals are guaranteed to be instantiated at first use by the C++ standard. However, I'm wondering what happens if I access a static local object while it is beeing constructed. I assume that this is UB. But what are the best practices to avoid this in the following situation?

A problem situation

The Meyers Singleton pattern uses a static local in a static getInstance() method to construct the object on the first use. Now if the constructor (directly or indireclty) calls getInstance() again, we face a situation where the static initialization is not yet completed. Here is a minimal example, that illustrates the problem situation:

class StaticLocal {
private:
    StaticLocal() {
        // Indirectly calls getInstance()
        parseConfig();
    }
    StaticLocal(const StaticLocal&) = delete;
    StaticLocal &operator=(const StaticLocal &) = delete;

    void parseConfig() {
        int d = StaticLocal::getInstance()->getData();
    }
    int getData() {
        return 1;
    }

public:
    static StaticLocal *getInstance() {
        static StaticLocal inst_;
        return &inst_;
    }

    void doIt() {};
};

int main()
{
    StaticLocal::getInstance()->doIt();
    return 0;
}

In VS2010, this works without problems, but VS2015 deadlocks.

For this simple, reduced situation, the obvious solution is to direclty call getData(), without calling getInstance() again. However, in more complex scenarios (as my actual situation), this solution is not feasible.

Attempting a solution

If we change the getInstance() method to work on a static local pointer like this (and thus abandon the Meyers Singleton pattern):

static StaticLocal *getInstance() {
    static StaticLocal *inst_ = nullptr;
    if (!inst_) inst_ = new StaticLocal;
    return inst_;
}

It is clear that we get an endless recursion. inst_ is nullptr on the first invokation, so we call the constructor with new StaticLocal. At this point, inst_ is still nullptr as it will only get assigned when the constructor finishes. However, the constructor will call getInstance() again, finding a nullptr in inst_, and thus call the constructor again. And again, and again, ...

A possible solution is to move the constructor's body into the getInstance():

StaticLocal() { /* do nothing */ }

static StaticLocal *getInstance() {
    static StaticLocal *inst_ = nullptr;
    if (!inst_) {
        inst_ = new StaticLocal;
        inst_->parseConfig();
    }
    return inst_;
}

This will work. However, I'm not happy with this situation, as a constructor should, well, construct a complete object. It is debateable if this situation can be made an exception, as it is a singleton. However, I dislike it.

But what's more, what if the class has a non-trivial destructor?

~StaticLocal() { /* Important Cleanup */ }

In the above situation, the destructor is never called. We loose RAII and thus one important distinguishing feature of C++! We are in a world like Java or C#...

So we could wrap our singleton in some sort of smart pointer:

static StaticLocal *getInstance() {
    static std::unique_ptr<StaticLocal> inst_;
    if (!inst_) {
        inst_.reset(new StaticLocal);
        inst_->parseConfig();
    }
    return inst_.get();
}

This will correctly call the destructor on program exit. But it forces us to make the destructor public.

At this point, I feel I'm doing the compiler's job...

Back to the original question

Is this situation really undefined behaviour? Or is it a compiler bug in VS2015?

What is the best solution to such a situation, prefably without dropping a full constructor, and RAII?

Answer 1

This leads to undefined behaviour by c++ 11 standard. The relevant section is 6.7:

If control enters the declaration concurrently while the variable is being initialized, the concurrent execution shall wait for completion of the initialization. If control re-enters the declaration recursively while the variable is being initialized, the behavior is undefined.

The example from the standard is bellow:

int foo(int i) {
    static int s = foo(2*i); // recursive call - undefined
    return i+1;
}

You are facing the deadlock since MSVC inserts mutex lock/unlock to make static variable initialization thread safe. Once you call it recursively, you are locking the same mutex two times in the same thread, what leads to dead lock.

This is how static initialization is implemented internally in llvm compiler.

The best solution IMO is to do not use singletons at all. Significant group of developers tend to think that singleton is anti-pattern. The issues like you mentioned is really hard to debug, because it occurs before main. Because order of globals initialization is undefined. Also, multiple translation units might be involved, so compiler won't catch this types of errors. So, when I faced the same problem in production code, I was obliged to remove all of the singletons.

If you still think that singleton is the right way to go, then you need to re-structurize your code somehow when your singleton object owns (holds them as members, for example) all the classes that calls GetInstance during the singleton initialization. Think of your classes like ownership tree, where the singleton is the root. Pass reference to parent, when you create a child, if child needs it.

Answer 2

The problem is that inside the class, you should be using "this" instead of calling getInstance, in particular:

void parseConfig() {
    int d = StaticLocal::getInstance()->getData();
}

Should simply be:

void parseConfig() {
    int d = getData();
}

The object is a singleton because the constructor is private and thus the user cannot construct an arbitrary number of objects. It's bad design to write the whole class assuming there will be only one instance of the object ever. At some point somebody may stretch the concept of a singleton like this:

static StaticLocal *getInstance(int idx) {
    static StaticLocal inst_[3];
    if (idx < 0 || idx >= 3)
      throw // some error;
    return &inst_[idx];
}

When that happens, it's much easier to update the code if there aren't calls to getInstance() throughout the class.

Why do changes like this happen? Imagine you were writing a class 20 years ago to represent the CPU. Of course there will only ever be one CPU in the system, so you make it a singleton. Then, suddenly, multi-core systems become commonplace. You still want only as many instances of the CPU class as there are cores in the system, but you won't know until the program is run how many cores are actually on a given system.

Moral of the story: Using the this pointer not only avoids recursively calling getInstance(), but future proofs your code as well.

Answer 3

Actually this code in its current form is stuck into 3-way infinite recursion. Hence it will never work.

getInstance() --> StaticLocal()
 ^                    |  
 |                    |  
 ----parseConfig() <---

To let it work, anyone of the above 3 methods has to compromise and come out of the vicious circle. You judged it right, parseConfig() is the best candidate.

Let's assume that all the recursive content of constructor is put into parseConfig() and non-recursive contents are retained in constructor. Then you may do following (only relevant code):

    static StaticLocal *s_inst_ /* = nullptr */;  // <--- introduce a pointer

public:
    static StaticLocal *getInstance() {
      if(s_inst_ == nullptr)
      {   
        static StaticLocal inst_;  // <--- RAII
        s_inst_ = &inst_;  // <--- never `delete s_inst_`!
        s_inst_->parseConfig();  // <--- moved from constructor to here
      }   
      return s_inst_;
    }   

This works fine.

Answer 4

One straight forward way of solving this is to separate the responsibilities, in this case "whatever StaticLocal is supposed to do" and "reading the configuration data"

class StaticLocal;

class StaticLocalData
{
private:
  friend StaticLocal;
  StaticLocalData()
  {
  }
  StaticLocalData(const StaticLocalData&) = delete;
  StaticLocalData& operator=(const StaticLocalData&) = delete;

  int getData()
  {
    return 1;
  }

public:
  static StaticLocalData* getInstance()
  {
    static StaticLocalData inst_;
    return &inst_;
  }
};

class StaticLocal
{
private:
  StaticLocal()
  {
    // Indirectly calls getInstance()
    parseConfig();
  }
  StaticLocal(const StaticLocal&) = delete;
  StaticLocal& operator=(const StaticLocal&) = delete;

  void parseConfig()
  {
    int d = StaticLocalData::getInstance()->getData();
  }

public:
  static StaticLocal* getInstance()
  {
    static StaticLocal inst_;
    return &inst_;
  }

  void doIt(){};
};

int main()
{
  StaticLocal::getInstance()->doIt();
  return 0;
}

This way, StaticLocal does not call itself, the circle is broken.

Also, you have cleaner classes. If you move the implementation of StaticLocal into a separate compile unit, users of static local won't even know that the StaticLocalData thingy exists.

There is a good chance that you will find that you do not need the functionality of StaticLocalData to be wrapped into a Singleton.