std::once_flag and std::call_once implementation

Question

I'm implementing std::once_flag and std::call_once for a particular MinGW build where they are not available, using only stuff available in the rest of the standard. The required semantics would be easy to implement with something like:

struct once_flag {
private:
 std::mutex _M_mutex;
 std::atomic_bool _M_has_run;
public:
 /// Default constructor
 once_flag() : _M_has_run(false) {}
 /// Deleted copy constructor
 once_flag(const once_flag&) = delete;
 /// Deleted assignment operator
 once_flag& operator=(const once_flag&) = delete;
 template<typename _Callable, typename... _Args>
 friend void
 call_once(once_flag& __once, _Callable&& __f, _Args&&... __args);
};
/// call_once
template<typename _Callable, typename... _Args>
void
call_once(once_flag& __once, _Callable&& __f, _Args&&... __args)
{
 // Early exit without locking
 if(__once._M_has_run) return;
 unique_lock<mutex> __l(__once._M_mutex);
 // Check again now that we locked the mutex
 if(__once._M_has_run) return;
 __f(std::forward<_Args>(__args)...);
 __once._M_has_runs = true;
}

(notice: I'm aware that stuff starting with __ and _[A-Z] is reserved for the standard library implementation; the point is that this code is meant to be the standard library implementation, so it's ok)

However, the once_flag constructor is required to be constexpr to make sure its initialization is thread-safe (or at least, so it seems from this mail about boost::once_flag); this complicates the matter, since std::mutex constructor is not constexpr. I came up with this kludge:

struct once_flag {
private:
 // Use a union to defer the initialization of the mutex to call_once
 union {
 std::mutex _M_mutex;
 char _M_dummy[sizeof(_M_mutex)];
 };
 // Spinlock used for mutex initialization
 std::atomic_flag _M_spinlock = ATOMIC_FLAG_INIT;
 std::atomic_int _M_run_status; // 0: not initialized; 1: mutex initialized; 2: function called
public:
 /// Default constructor
 constexpr once_flag() noexcept : _M_run_status(0), _M_dummy({0}) {}
 /// Deleted copy constructor
 once_flag(const once_flag&) = delete;
 /// Deleted assignment operator
 once_flag& operator=(const once_flag&) = delete;
 ~once_flag() {
 if(_M_run_status!=0) {
 _M_mutex.~mutex();
 }
 }
 template<typename _Callable, typename... _Args>
 friend void
 call_once(once_flag& __once, _Callable&& __f, _Args&&... __args);
};
/// call_once
template<typename _Callable, typename... _Args>
void
call_once(once_flag& __once, _Callable&& __f, _Args&&... __args)
{
 // If the function has already run, quit immediately
 if(__once._M_run_status == 2) return;
 // We may not have the mutex yet
 if(__once._M_run_status == 0) {
 // Acquire the spinlock
 while (__once._M_spinlock.test_and_set(std::memory_order_acquire));
 // Check again; we may be the *second* thread to acquire the spinlock
 if(__once._M_run_status==0) {
 // Initialize the mutex
 new(&__once._M_mutex) std::mutex;
 __once._M_run_status = 1;
 }
 // Release the spinlock (std::mutex constructor is noexcept, so there's no
 // need for fancy RAII)
 __once._M_spinlock.clear(std::memory_order_release);
 }
 // Now that we are sure we have a mutex, acquire it
 unique_lock<mutex> __l(__once._M_mutex);
 // Check again; we may be the *second* thread to acquire the mutex
 if(__once._M_run_status == 2) return;
 // Call __f
 __f(std::forward<_Args>(__args)...);
 // All done
 __once._M_run_status = 2;
}

The basic idea is to use a union to defer the actual creation of the std::mutex to call_once.

The std::atomic_int _M_run_status member marks the progress in the process: 0 means that we didn't even construct the mutex, 1 that we have the mutex but the function was not called yet, 2 that we called the function.

The call_once has an early exit for the "typical" case (= the function has already run) and uses a crude spinlock to make sure only one thread initializes the mutex. The idea is that the std::mutex initialization should be really fast (so a spinlock with no sleep should work fine), while the function call may be as long as the client code want, so an std::mutex (which should defer to the OS the yielding) is definitely more appropriate.

From a quick glance I think I got the obvious gotchas covered, my main concern is that this feels a bit too much complicated, and it somewhat does its job twice - first it has to assure that the mutex initialization occurs only once, then the same has to be done to the function.

The only portable alternative that readily comes to mind is to ditch std::mutex and just use a spinlock with a std::this_thread::yield() instead. That would remove all the union/std::mutex initialization kludge, but would add a dependency to the <thread> header (which may not be ok if it in turn has to include <mutex>).

• user24597: Look, one point. According to en.cppreference.com/w/cpp/thread/mutex/mutex the std::mutex constructor is constexpr since C++11. Why did you say since std::mutex constructor is not constexpr ? You can add constexpr to the first version of struct once_flag and it is compiles. I think you must have said since std::mutex constructor is not constexpr in MinGW

Answer 1

Firstly, I'm not 100% sure how closely you want to follow the standard, but this is missing a (potentially significant) piece: what to do if the selection function throws an exception. From cppreference.com:

If the selected function exits via exception, it is propagated to the caller. Another function is then selected and executed.

Fixing this should simply be a matter of the following:

try {
 __f(std::forward<_Args>(__args)...);
} catch(...) {
 throw; 
}

The rest of the mechanisms in your code should then select another function to run, as is required.

As for the concurrency aspect, this should be fine. Since this is for MinGW, which runs on Windows, you can be pretty sure this will be used on an x86/64 processor. Because the x86/64 memory model has release/acquire semantics baked into it, the generated code shouldn't even have any memory fences (e.g. MOV is sufficient), so you can be pretty sure about its safety.

(If you happen to be running Itanium or ARM for whatever reason, it should still be fine, however).

You might also want to have a look at invoke. This would allow someone to use (say) a pointer to member function without having to wrap it in a call to bind or a lambda. Boost seems to support this, whether std::call_once does as well, I'm not sure.

• Matteo Italia: Thanks for your reply! 1. Isn't it already ok? The flag variable is set to 2 only after __f is called, so if an exception is thrown the flag remains to 1, the mutex is released (it was locked through a unique_lock, which has RAII semantics) and the next thread which was waiting on it is allowed to give __f another try.
• Matteo Italia: 2. Invoke: it may be a nice addition, but it isn't required by the standard, correct? (actually, I'm a bit wary of adding extensions that give meaning to code which wouldn't compile, since in these days of SFINAE they can effectively alter the observable behavior of valid code).
• Yuushi: @MatteoItalia Yes, you're right for point 1, my apologies. For point 2, I'm actually not sure if it is required by the standard or not - although the second answer here suggests it might be (the quote from Section 30.4.4.2 Function call-once).
• Matteo Italia: I checked the standard, it seems like it is actually required; thank you for spotting it, I'll go fix it!
• Matteo Italia: (also, it seems like some DECAY_COPY is involved, I'll have to look up what they mean)

Answer 2

Just a few basic suggestions on code improvement, hopefully someone else will be able to give you a few hints on the concurrency aspects of the code later.

• Please drop the __ prefixing. I know you wanted to make this look like code from the std library, but the truth is that code in the library headers is obfuscated on purpose. Prefixing everything with two underscores makes the code look soo ugly. There's no reason to do that besides intentional obfuscation, and I'm sure you want your code to be programmer-friendly and maintainable.

• This is begging for an enum:

  std::atomic_int _M_run_status; // 0: not initialized; 1: mutex initialized; 2: function called
  

  Old-style enums convert implicitly to integer, so you can use one of those:

  enum RunState {
   NotInitialized,
   MutexInitialized,
   FunctionCalled
  };
  

• Trim down your comments a little bit. Some are completely pointless, like // Default constructor, others are just repeating what can be clearly figured by just reading the code: // Initialize the mutex, // Call __f...

• I would put all that mutex initialization code inside a separate helper method, since it is fairly complicated and distracts from the whole point of call_once which is just running a callback:

  call_once(once_flag& once, Callable&& func, Args&&... args)
  {
   if (once.m_run_status == FunctionCalled) {
   return;
   }
   // A private member that friend call_once can access
   once.check_mutex_initialized(); 
   std::unique_lock<mutex> lock(once.m_mutex);
   // Check again; we may be the *second* thread to acquire the mutex
   if (once.m_run_status == FunctionCalled) {
   return;
   }
   func(std::forward<Args>(args)...);
   once.m_run_status = FunctionCalled;
  }
  

• Matteo Italia: First of all, thank you for taking the time to respond, although my actual concern was actually the concurrency aspect and the respondence to the required standard semantic. To address the points you made:
• Matteo Italia: 1. the point is that this code is standard library, as I'm patching the MinGW headers. The code in the standard library isn't "obfuscated" (what kind of obfuscation is adding some underscores before identifiers anyway?). The standard explicitly reserves identifiers starting with _[A-Z] at the global scope and identifiers starting with __[a-z] in all scopes (or something like that) explicitly for library implementation details, so that anything written by library users before including standard headers shouldn't harm them. So, yes, it's ugly but is pretty much required.
• Matteo Italia: 2. agreed, I used an atomic_int because I'm not sure about the presence/guarantees of std::atomic<T> over an enum, but it's true that I can use the names just as constants. They'll definitely have to keep the ugly __ prefix, though.
• Matteo Italia: 3. The comments above constructors are actually a redundant, they are here because they were in the original gthreads-based implementation of this class from MinGW which I used as a starting base. The mutex intialization comment is here because of the odd placement new syntax, although it could be reworded to explicitly address this issue. For the rest, the algorithmic part has been heavily commented on purpose while explaining it to a coworker to address some of his doubts; I think that when dealing with delicate thread-related code it's better to be really explicit.
• glampert: @MatteoItalia Well I suspect some of the "ugliness" of the std library code is intentional, even though never stated explicitly, probably in an attempt to discourage people from copying it. Related thread: stackoverflow.com/questions/4180166/…