6
\$\begingroup\$

I have implemented a thread safe holder to safely pass data between threads.

User can set value many times, but only the first SetIfEmpty call stores the value, then user may read the value many times.

template <typename T>
class ThreadSafeHolder {
public:
    ThreadSafeHolder() : is_value_set_(false) {
    }

    void SetIfEmpty(const T& value) {
        std::lock_guard<std::mutex> lock(mutex_);
        // memory_order_relaxed is enough because storing to
        // `is_value_set_` happens only in `SetIfEmpty` methods
        // which are protected by mutex.
        if (!is_value_set_.load(std::memory_order_relaxed)) {
            new(GetPtr()) T(value);
            is_value_set_.store(true, std::memory_order_release);
        }
    }

    void SetIfEmpty(T&& value) {
        std::lock_guard<std::mutex> lock(mutex_);
        if (!is_value_set_.load(std::memory_order_relaxed)) {
            new(GetPtr()) T(std::move(value));
            is_value_set_.store(true, std::memory_order_release);
        }
    }

    //! This method might be safely call only if previous `IsEmpty()`
    //! call returned `false`.
    const T& Get() const {
        assert(!IsEmpty());
        return *GetPtr();
    }

    bool IsEmpty() const {
        // memory_order_acquire loading to become synchronize with
        // memory_order_release storing in `SetIfEmpty` methods.
        return !is_value_set_.load(std::memory_order_acquire);
    }

    ~ThreadSafeHolder() {
        if (!IsEmpty()) {
            GetPtr()->~T();
        }
    }

private:
    T* GetPtr() {
        return reinterpret_cast<T*>(value_place_holder_);
    }

    const T* GetPtr() const {
        return reinterpret_cast<const T*>(value_place_holder_);
    }

    // Reserved place for user data.
    char value_place_holder_[sizeof(T)];
    // Mutex for protecting writing access to placeholder.
    std::mutex mutex_;
    // Boolean indicator whether value was set or not.
    std::atomic<bool> is_value_set_;
};

Questions

  • Is the code correct in general?
  • Is access to is_value_set_ member properly synchronized?
  • Might be access to is_value_set_ member even more relaxed?

Application

I wanted to develop such holder to pass active exceptions from worker threads to main thread.

Main thread:

ThreadSafeHolder<std::exception_ptr> exceptionPtrHolder;
// Run many workers.
// Join workers.
if (!exceptionPtrHolder.IsEmpty()) {
    std::rethrow_exception(exceptionPtrHolder.Get());
}

Worker thread:

try {
    while (exceptionPtrHolder.IsEmpty()) {
        // Do hard work...
    }
} catch (...) {
    exceptionPtrHolder.SetIfEmpty(std::current_exception());
}

Note about std::promise

std::promise is not suitable here (despite the fact that std::promise::set_value is thread safe) because

An exception is thrown if there is no shared state or the shared state already stores a value or exception.

\$\endgroup\$
  • 1
    \$\begingroup\$ You forgot to implement the "Rule of Three/Five" \$\endgroup\$ – Martin York Sep 27 '15 at 3:17
  • \$\begingroup\$ Does value_place_holder_ need to be allocated statically? \$\endgroup\$ – cr_oag Sep 29 '15 at 18:59
  • \$\begingroup\$ @LokiAstari thanks for pointing, I have added move constructor and move assignment operator. \$\endgroup\$ – user2932661 Sep 29 '15 at 20:28
  • \$\begingroup\$ @cr_oag I agree, a place for T value might be allocated dynamically, but then SetIfEmplty might throw. But if value_place_holder_ is allocated in advance and T constructor or move constructor does not throw then SetIfEmpty does not throw. \$\endgroup\$ – user2932661 Sep 29 '15 at 20:35
  • \$\begingroup\$ @user2932661 The difference would be that ThreadSafeHolder would throw a std::bad_alloc exception in its constructor if it is unable to reserve enough memory to hold T, SetIfEmpty() would not throw, assuming that its constructors don't throw either. Is that acceptable? \$\endgroup\$ – cr_oag Sep 29 '15 at 20:39
3
\$\begingroup\$

Code looks good. I only have a few comments.

Alignment

Our data is stored thusly:

char value_place_holder_[sizeof(T)];

This has alignment 1. You really want it to have the same alignment as T. That's easy enough:

using Storage = std::aligned_storage_t<sizeof(T), alignof(T)>;
Storage data;

Usability

Right now, you have two setting functions:

void SetIfEmpty(const T& value) { ... }
void SetIfEmpty(T&& value) { ... }

But if you're already constructing using placement new, we could just allow for arbitrary arguments. It'll let your users do more with what you have:

template <typename... Args>
void SetIfEmpty(Args&&... args) {
    ...
    new (GetPtr()) T(std::forward<Args>(args)...);
    ...
}

That'll also reduce the code duplication on your copy and move construction.

Unnecessary Mutex

You have a std::atomic<bool>, let's take advantage of it. There's a function called compare_exchange_strong with which we can do:

template <typename... Args>
void SetIfEmpty(Args&&... args) {
    bool expected = false;
    if (is_value_set_.compare_exchange_strong(expected, true))
    {
        new (GetPtr()) T(std::forward<Args>(args)...);
    }
}

That is a single atomic operation. We load the value of is_value_set_ and if it was false, we set it to true and the function returns true. If two threads get to that line at the same time, only one of them will succeed in flipping it to true and the other will fail.

This also gives us...

Copyable? Movable?

Dropping the mutex means our class is tentatively copyable and movable. Do we want to support that? If so, the default behavior is bitwise copy and move which is fine for PODs but not for anything else. You would have to add those operations.

\$\endgroup\$
  • 1
    \$\begingroup\$ What if the placement new throws an exception? The invariants of the ``ThreadSafeHolder` class are broken, since is_value_set_ is true, but the storage is still uninitialized. Therefore, setting the atomic variable must be rolled back. Hence, if other threads try to do the same at the same time, then they have to wait to see whether the operation succeeded, so they can do the job, if the first one to try failed. Therefore, a mutex is probably necessary. This can still be optimized by double-checked locking. In short: The code does not seem to be corrects as of now. \$\endgroup\$ – Ralph Tandetzky Sep 26 '15 at 15:44
  • 1
    \$\begingroup\$ If the compare/exchange takes place, the thread could pause before the new happens, and a second thread calling Get will fail. \$\endgroup\$ – JS1 Sep 26 '15 at 20:52
  • \$\begingroup\$ Thanks for you comment, I have incorporated your all remarks, except removing mutex because of the reasons stated above. \$\endgroup\$ – user2932661 Sep 29 '15 at 20:38

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.