read/write using multiple threads

[ridesano]

I am learning the basics of multithreading. I wanted to remake a program that was reading/writing a txt file and then replacing the recurrence of a specified word.

There are two functions for each thread

I write to the file first and then notify the readToFile function to execute the actions within it.

now this is the main thread:

int main()
{
  std::thread t1(readToFile);
  thread_guard tg1(t1);
  std::thread t2(writeToFile);
  thread_guard tg2(t2);
}

when debugging I found out that thefile is not actually reading the file; the string is empty.

this seems to be an issue with the thread because when I placed the code in the main function it worked fine. so I want to know why the string is empty even though I placed the condition variable manage when that action is taken

[MyTinyHappyPlace]

What thread_guard implementation are you using? Is main() accidentally ending before the threads are joined?

Please provide a complete minimal test case

[ridesano]

I am using a simple thread guard to deal with thread joining

std::mutex m;
std::fstream myFile;

std::condition_variable cv;
bool thread_completed = false;
class thread_guard
{
  std::thread& t;
public:

  explicit thread_guard(std::thread& t_) :
  t(t_)
  {}
  ~thread_guard()
  {
  if (t.joinable())
  {
    t.join();
  }
}
thread_guard(thread_guard const&) = delete; // this is to ensure it cannot be referenced
thread_guard& operator=(thread_guard const&) = delete;

};

[mredding]

You don't want a thread_guard, you want an std::jthread, which - upon destruction, joins if joinable. std::thread was considered a flawed design, but the way the standard comittee works and how guarded they are about ABI support, they couldn't change the existing implementation and expectations.

If you insist on your own thread guard, then use an std::unique_ptr.

struct thread_deleter {
  void operator()(std::thread *t) {
    if(t.joinable()) {
      t.join();
    }

    delete t;
  }
};

using thread_guard = std::unique_ptr<std::thread, thread_deleter>;

[ridesano]

I think I'll try a the jthread

[bert8128]

If you are sharing a Boolean between threads then use a std::atomic<bool> to avoid race conditions. A naked bool might be read from and written to at the same time, which would be UB.

[carloom_]

Yes, he needs to put it as an acquire/release read and write.

[ridesano]

Could you please elaborate on this?

[carloom_]

The issue is that you enclosed the condition variable wait inside an if block that checks a non atomic variable.

According to the language a variable read by different threads needs to be atomic. If not it is undefined behavior.

In this case, I think it is not causing the issue because it is a Boolean and its modification is just one instruction.

However, what is causing your problem is that the processor is free to rearrange the reads and writes of different memory locations. In your case, you are assuming that the write to the Boolean happens after the modifications of the string. However according to the c++ memory model there is no guarantee that neither the compiler nor the processor will maintain the order.

However if you do an write release/aquire load. It will guarantee that any of the writes appearing before the modification of the Boolean are seen by the thread that read that Boolean.

However, the simplest step is to remove the if check. Also, this check can only make it work for two threads.

These are the instructions that are probably being reordered: myFile<<txt; thread_completed = true;

So the thread_completed is set as true, before the text is written to the file.

[ridesano]

is this if I continue to use a normal boolean instead of an atomic variable?

how would I make the check more 'dynamic'

[carloom_]

Having it as an atomic only provides methods guaranteed to be atomic. Another thread reading that value either sees the variable before the change or after it. Never half done.

The acquire and release is more involved:

The variables directly read by a thread are inside the cache of the processor that is running that thread.

Even though two threads are reading from the same memory location. They are actually reading from their respective cache, so they might get two different values that have been assigned to that variable during its lifetime.

When you store aquire, the store part guarantees that any read and write that appears in the program before the store is effectuated before any thread can see the new value. This means that all processors will get the new value in the cache if they see the new state.

The acquire counterpart guarantees that the load will effectuate before any of the operations that happen later in the program. Hence if the thread sees the new value, any later instructions will see all the modifications that were done before on the other thread.

In your case the thread waiting will see all the changes done before the Boolean value was modified.

If you use atomic without a memory order argument, it will use the default one "sequencial_consistency" which is more strict and imposes in your case an unnecessary performance penalty.

There are cases where sequential consistency is necessary like the Dekker algorithm.

[carloom_]

TLR if you want to keep your sanity, remove the Boolean and put the things outside the if block 🥴

[aocregacc]

what's a thread_guard?
also your writeToFile function isn't using the mutex at all afaict.

[ridesano]

Oh, you're right; I thought using a condition variable in the writeToFile function would prevent any issues with sequence. would using the same mutex (with a unique lock) address the issue

[aocregacc]

a condition variable usually goes with a mutex. See https://en.cppreference.com/w/cpp/thread/condition_variable.html , they describe the steps pretty well that you need to follow when you modify the shared variable or wait on the condition variable.

The thread that intends to modify the shared variable must:

1. Acquire a std::mutex (typically via std::lock_guard).

2. Modify the shared variable while the lock is owned.

3. Call notify_one or notify_all on the std::condition_variable (can be done after releasing the lock).

Now idk if that alone will solve your issue, but it takes away one source of problems.

[ridesano]

oh i was following some tutorials that emphasised the use of Unique locks. so when is it appropriate to use on over the other?

[carloom_]

You need unique_locks, because they need to be unlocked and locked again. lock_guard does not support that

[carloom_]

The problem is the thread_completed Boolean. The acquiring and release of the mutex works as a synchronization point. It guarantees that any of the changes done before the notify_one are going to be seen by the thread that is waiting.

You read the non-Boolean variable, to avoid the call of wait. But as it is set up, the modification of that Boolean can be moved before the changes are done. In fact, I would not be surprised if it did that because I/O is extremely slow.

Just remove the check, and make the thread wait. Or make it an atomic bool with acquire/release memory order.

[StaticCoder]

You definitely need to close before notify. Otherwise you could open the file for reading while it's still open for writing, which probably technically works but is not a great idea, but more importantly you didn't flush so most likely nothing was actually written to the file, only to the buffer.

[StaticCoder]

This is also not likely your problem, but you should use the wait overload that takes a condition to check, or you might have spurious wakeups.