Is this really thread safe?

[MightTower]

Hello, this is more a question than a de facto issue. Is it really thread safe if this code from signal.hpp

// lock the mutex for writing
std::lock_guard<std::mutex> lock(m_write_targets_mutex);
.
.
.
// replace the pointer to the targets (in a thread safe manner)
m_targets = new_targets;

is executed and in another thread (B) for example:

if(auto t = m_targets) {
.
.
.
}

is executed. Could it be a problem that m_targets is not locked in the thread B? Maybe I am overlooking something.

Would be great if someone could explain why this is thread save.

Thanks

[MightTower]

I think I got it, since auto t = m_targets only copies the pointer its either the old one or the new one in case that m_targets = new_targets; is excuted simultaneously in another thread. Is this the point why this is thread safe?

[sergey-shambir]

Is this the point why this is thread safe?

This library is not thread safe. At first, memcpy of the pointer is thread-safe only on some platforms At second, instructions re-ordering is still possible. atomic_shared_ptr from C++20 can help, but currently library is not thread-safe

[Kosta-Github]

All write operations to m_targets are protected by the m_targets_write_mutex, therefore it should be thread safe... Can you point me to a function/method where modifying m_targets is not protected by the corresponding mutex?

[sergey-shambir]

It's not enough to protect writes since shared_ptr isn't atomic. Some CPUs guarantue atomic reads/writes for pointers, but

• compiler still can re-order reads
• CPU can re-order reads
• on some CPUs, read can mix two values if another thread writes new value at the same time See also "20.8.2.5 shared_ptr atomic access" in C++14 or C++17 standard draft.

[sergey-shambir]

Protecting writes to shared_ptr is like protecting writes to int: unprotected reads still can introduce data races.

[Kosta-Github]

@sergey-shambir this should solve the issue then, right? https://github.com/Kosta-Github/signals-cpp/commit/15ef8dd2a23ecd693268b99974f571e0eee4993e

[sergey-shambir]

Yes, it should