Skip to main content
Rollback to Revision 1
Source Link
Ethan Bierlein
  • 15.8k
  • 4
  • 59
  • 146

EDIT:

Incorporating remarks from the comments:

It is much better to have the same alignment as T:

using ValueStorage = std::aligned_storage<sizeof(T), alignof(T)>;
ValueStorage value_place_holder_;

Turn two SetIfEmpty methods to one:

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

To follow the rule of three/five/zero I have added move constructor and move assignment operator:

ThreadSafeHolder(ThreadSafeHolder&& other)
    : is_value_set_(other.is_value_set_.load())
{
    if (!other.IsEmpty()) {
        new (GetPtr()) T(std::move(other.Get()));
    }
}

ThreadSafeHolder& operator=(ThreadSafeHolder&& other) {
    if (&other != this) {
        DestroyValueIfNecessary();
        if (!other.IsEmpty()) {
            new (GetPtr()) T(std::move(other.Get()));
        }
        is_value_set_ = other.is_value_set_.load();
    }
    return *this;
}

The full code is available on https://gist.github.com/anonymous/c5d987abfc73eda11e71


EDIT:

Incorporating remarks from the comments:

It is much better to have the same alignment as T:

using ValueStorage = std::aligned_storage<sizeof(T), alignof(T)>;
ValueStorage value_place_holder_;

Turn two SetIfEmpty methods to one:

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

To follow the rule of three/five/zero I have added move constructor and move assignment operator:

ThreadSafeHolder(ThreadSafeHolder&& other)
    : is_value_set_(other.is_value_set_.load())
{
    if (!other.IsEmpty()) {
        new (GetPtr()) T(std::move(other.Get()));
    }
}

ThreadSafeHolder& operator=(ThreadSafeHolder&& other) {
    if (&other != this) {
        DestroyValueIfNecessary();
        if (!other.IsEmpty()) {
            new (GetPtr()) T(std::move(other.Get()));
        }
        is_value_set_ = other.is_value_set_.load();
    }
    return *this;
}

The full code is available on https://gist.github.com/anonymous/c5d987abfc73eda11e71

Incorporating remarks from the comments
Source Link

EDIT:

Incorporating remarks from the comments:

It is much better to have the same alignment as T:

using ValueStorage = std::aligned_storage<sizeof(T), alignof(T)>;
ValueStorage value_place_holder_;

Turn two SetIfEmpty methods to one:

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

To follow the rule of three/five/zero I have added move constructor and move assignment operator:

ThreadSafeHolder(ThreadSafeHolder&& other)
    : is_value_set_(other.is_value_set_.load())
{
    if (!other.IsEmpty()) {
        new (GetPtr()) T(std::move(other.Get()));
    }
}

ThreadSafeHolder& operator=(ThreadSafeHolder&& other) {
    if (&other != this) {
        DestroyValueIfNecessary();
        if (!other.IsEmpty()) {
            new (GetPtr()) T(std::move(other.Get()));
        }
        is_value_set_ = other.is_value_set_.load();
    }
    return *this;
}

The full code is available on https://gist.github.com/anonymous/c5d987abfc73eda11e71


EDIT:

Incorporating remarks from the comments:

It is much better to have the same alignment as T:

using ValueStorage = std::aligned_storage<sizeof(T), alignof(T)>;
ValueStorage value_place_holder_;

Turn two SetIfEmpty methods to one:

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

To follow the rule of three/five/zero I have added move constructor and move assignment operator:

ThreadSafeHolder(ThreadSafeHolder&& other)
    : is_value_set_(other.is_value_set_.load())
{
    if (!other.IsEmpty()) {
        new (GetPtr()) T(std::move(other.Get()));
    }
}

ThreadSafeHolder& operator=(ThreadSafeHolder&& other) {
    if (&other != this) {
        DestroyValueIfNecessary();
        if (!other.IsEmpty()) {
            new (GetPtr()) T(std::move(other.Get()));
        }
        is_value_set_ = other.is_value_set_.load();
    }
    return *this;
}

The full code is available on https://gist.github.com/anonymous/c5d987abfc73eda11e71

Source Link

Thread safe holder

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.