Use atomic instead of CriticalSection?

Question

I have a code that gives a some data, Do some operation on files and etc. I Use 1 thread for giving data and 4 thread for reading and searching inside of data.

The 4 Threads use a vector of files for searching (Using the same function). For avoiding of synchronization problems (All threads Read the same file in the same time) I use CriticalSection() WinAPI:

void ReadData(char* fileName)
{
   EnterCriticalSection(&CriticalSection);

   // Open file

   // Read file data
   std::vector<std::string> data;

   ... Find data inside file

   // Close file

   LeaveCriticalSection(&CriticalSection);
}

But I saw this Post And this:

Objects of atomic types are the only C++ objects that are free from data races; that is, if one thread writes to an atomic object while another thread reads from it, the behavior is well-defined.

My question is: It's better idea to use std::atomic instead of CriticalSection? or I miss understanding about the usage of atomics.

if memory access can be done as interlocked no need use critical section or other synchronizations here. however far from everything can be done with interlocked operations only. you need understand what you doing and what you need — RbMm, Oct 27 '17 at 12:08
Btw critical sections are broken on Windws as locking can fail with SEH exception. You should consider using SRW locks introduced in NT 6.x (since Vista). They take less space and can be initialized statically. Starting with Visual Studio 2015 `std::mutex` is based on SRW locks. In previous versions `std::mutex` is hugely broken and can cause deadlocks. For example if you call `std::mutex::lock()` for the first time from `DllMain` it will deadlock. — , Oct 27 '17 at 12:14
@Ivan - critical sections not broken on Windows and locking never fail or cause exception — RbMm, Oct 27 '17 at 12:19
*std::mutex::lock() for the first time from DllMain it will deadlock* - really ? always and unconditionally ? deadlock cause not mutex. deadlock cause illiteracy. if wait on "critical section" (in wide sense) inside another critical section - of course can be deadlock. but this not mean that something broken. absolute no — RbMm, Oct 27 '17 at 12:25
@RbMm seems they have fixed critical sections at some point by adding some fallback path. Still SRW locks are the only possible implementation for `std::mutex` for C++11 (as you need its constructor to be `constexpr`, which is still broken at least on VS2015). As for deadlock: yes, always and unconditionally. And I know that what causes the deadlock, the problem is that nobody expects mutex locking to lock another additional mutex internally. So yes, `std::mutex` is broken on VS pre-2015. So are conditional variables. — , Oct 27 '17 at 12:35
@Ivan - critical sections is very old in windows and it internal implementation many times changed. but i never listen about some problems with it. and it never raise any exception. and about *yes, always and unconditionally.* - this is false. *nobody expects mutex locking to lock another additional mutex internally* - nonsense. can you explain on which object wait thread that cause deadlock (how you say) ? how loader critical section related to this ? — RbMm, Oct 27 '17 at 12:40
@RbMm - I think he is talking about this: [On the gradual improvements in how the system deals with the failure to initialize a critical section](https://blogs.msdn.microsoft.com/oldnewthing/20171020-00/?p=97256) — ssbssa, Oct 27 '17 at 12:52
@ssbssa - this is under most extreme case when so low memory in windows that even event object can not be created. think almost never was in practic. and for mutex locking inside loader lock * always and unconditionally* of course no sense and not true — RbMm, Oct 27 '17 at 12:56
@RbMm `std::mutex` implementation in VS2013 and earlier based on some "Concurrency" runtime which is initialized lazily. Basically it will try to do some tricky initialization and will hang somewhere in a "RegisterWaitForSingleObject". Keep in mind that this all can be done with a single thread in a process and a mutex created on stack - there is not deadlock in user code. — , Oct 27 '17 at 12:58
You can fail to allocate kernel object not only because of low memory situation, but because of limit on how much HANDLE's process can have. I am not sure what are current limits are, but if your program creates many kernel objects you might meet a failure even if you have lots of free memory. Now with what @ssbssa says it looks like they have fixed critical sections in Vista. — , Oct 27 '17 at 13:01
@Ivan - yes, every process have limit (very high) for handles, memory etc (based on quota) but this is very very rarely case. and implementation many time is changed. begin from vista used not event but common keyed event in windows. begin from 8.1 use `NtWaitForAlertByThreadId` which not create additional objects — RbMm, Oct 27 '17 at 13:14
@RbMm I have checked on Windows 7 and they still call NtCreateEvent internally, so it looks like they've just added a fallback path, but still use events in most situations. — , Oct 27 '17 at 13:19
@Ivan - yes, in win7 create event. if this is fail - try use keyed event — RbMm, Oct 27 '17 at 13:20
`std::mutex` isn't broken. As Microsoft states in [DLL best practices](https://msdn.microsoft.com/en-us/library/windows/desktop/dn633971(v=vs.85).aspx), inside DllMain you should never *"Synchronize with other threads. This can cause a deadlock.*". That's because the very basic APIs such as GetModuleFileName acquire the loader lock, which DllMain already holds. — rustyx, Oct 27 '17 at 13:39
@Ivan on win7 really was [deadlock](https://connect.microsoft.com/VisualStudio/feedback/details/809005/deadlock-when-locking-std-mutex-during-dllmain-static-initialization) if call `RegisterWaitForSingleObject` before process finish initialize (before exe entry point), because this api wait for working thread start, but working thread wait for process initialization complete.. this bug fixed in win8.1. but anyway not unconditional (if dll load later, when process initialized - no deadlock). but src - mistake try wait inside loader lock — RbMm, Oct 27 '17 at 14:22

Werner Erasmus · Answer 1 · 2017-10-27T13:19:42.490

std::atomic is not overloaded for a vector. It is also not copyable or movable (from here and here), and therefore one is not able to use it as a std::vector value_type (or one might need to provide your own specialization, which I have not (and won't) attempt/ed).

However, if you want your code to be platform independent, std::mutex would do perfectly e.g:

std::mutex myMutex_; //Typically a member or static or in unnamed namespace

void ReadData(char* fileName)
{
   std::lock_guard<std::mutex> guard(myMutex_);
   // Open file

   // Read file data
   std::vector<std::string> data;

   ... Find data inside file

   // Close file

   //... Releases when scoped left...
}

In a certain sense my answer echos this previous post.

score 3 · Accepted Answer · edited Jun 20 '20 at 09:12

Use standard C++

In your code, you use EnterCriticalSection() and LeaveCriticalSection() functions of Microsoft's WinAPI.

These have wto major inconveniences: first they are not portable, second they are not safe: what would happen if an exception would cause the thread to leave ReadData() in an unexpected fashion ? You might end-up with a critical section that appears to windows not to be left, starving all the other threads !

The standard C++ alternative using a lock_guard on a mutex, as demonstrated by Werner, is much safer: first it is portable across platforms, but in addition, it implements the RAII idiom, which ensures that if there's an unexpected exception, the lock_guard is destroyed when the function is left, causing the mutex to be released.

Use of atomic might not be sufficient

Very often, people are tempted to use atomics because they avoid data races and give the impression that it will address all the thread synchronisation issues.

Unfortunately, this is not true. As soon as you use several atomics, you might make assumption about the overall consistency, whereas in reality things could happen differently and cause very nasty bugs. Creating lock-free algorithms and data-structures is extremely challenging and difficult. I therefore recommend strongly to read Anthony William's excellent book "C++ concurrency in action" : he explores in full depth all the related aspects.

Other alternatives

In your question you refer to a vector. Maintaining a concurrent thread safe vector is very difficult, because basically, whenever the vector's capacity has to be extended, a reallocation might occur, invalidating all the iterators and pointers to this vector, wherever they are used. Fortunately, there are some thread safe implementations like Microsoft's Parallel Pattern Library.

On the other hand, if you use a vector only to store the lines of the file and process them sequentially, you mays as well consider using a queue, with the advantage of being able to use one of the many thread safe implementations available, such as for example boost.

Thank you, First: I Also used `CreateThread` for multithreading it's handier for me when I want to stop, pause or resume the threads. Second: What do you mean by _queue_? What is the good container instead of a vector? — Ali Sepehri-Amin, Oct 27 '17 at 20:03
Yes, I should have mentioned the benefit of scoped locking in terms of exception handling. Thoroughly explained — Werner Erasmus, Oct 28 '17 at 05:49

Use atomic instead of CriticalSection?

2 Answers2

Use standard C++

Use of atomic might not be sufficient

Other alternatives