8
\$\begingroup\$

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>).

\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – Sergey Kurenkov Dec 8 '16 at 8:27
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Matteo Italia Jan 25 '16 at 7:20
  • \$\begingroup\$ 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). \$\endgroup\$ – Matteo Italia Jan 25 '16 at 7:23
  • \$\begingroup\$ @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). \$\endgroup\$ – Yuushi Jan 25 '16 at 8:25
  • \$\begingroup\$ I checked the standard, it seems like it is actually required; thank you for spotting it, I'll go fix it! \$\endgroup\$ – Matteo Italia Jan 25 '16 at 12:19
  • \$\begingroup\$ (also, it seems like some DECAY_COPY is involved, I'll have to look up what they mean) \$\endgroup\$ – Matteo Italia Jan 25 '16 at 12:26
4
\$\begingroup\$

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;
    }
    
\$\endgroup\$
  • \$\begingroup\$ 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: \$\endgroup\$ – Matteo Italia Jan 21 '16 at 19:04
  • \$\begingroup\$ 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. \$\endgroup\$ – Matteo Italia Jan 21 '16 at 19:07
  • \$\begingroup\$ 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. \$\endgroup\$ – Matteo Italia Jan 21 '16 at 19:10
  • \$\begingroup\$ 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. \$\endgroup\$ – Matteo Italia Jan 21 '16 at 19:16
  • \$\begingroup\$ @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/… \$\endgroup\$ – glampert Jan 21 '16 at 19:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.