5
\$\begingroup\$

I'm going to turn them (multi-threading) to the first function after the end of the operation to notify the second function that has already finished work and the second function it received through multi-threading.

#include "stdafx.h"
#include <cstdint>
#include <iostream>
#include <mutex>
#include <queue>
#include <condition_variable>
#include <fstream> 
#include <sstream>
#include <algorithm>
std::mutex mutex;
std::queue<std::string> queue;
std::condition_variable conditionVariable;

void ProduceData()
{
   const char* pathToFile = "D:\\text.txt";
   std::ifstream stream(pathToFile);
   if (!stream)
   {
      std::cout << "Can not open file\n";
      return;
   }

   const std::uint8_t maxWordCount = 5;
   std::string word;
   while (stream >> word)
   {
      std::lock_guard<std::mutex> lockGuard(mutex);
      for (std::uint8_t count = 0; (count < maxWordCount) || (count != '\0'); ++count)
        queue.push(word);
      conditionVariable.notify_one();
   }
 }


 void ConsumeData()
 {
   while (true)
   {
    std::unique_lock<std::mutex> uniqueLock(mutex);
    conditionVariable.wait(uniqueLock, [] {return !queue.empty(); });
    while (!queue.empty())
    {
        const std::string& str = queue.front();
        std::size_t numVowels = std::count_if(str.begin(), str.end(), isVowel);
        std::cout << str << "-" << numVowels;
        queue.pop();
    }
    uniqueLock.unlock();
   }
}




int main()
 {
std::thread t1(ProduceData);
std::thread t2(ConsumeData);
t1.join();
t2.join();

  return 0;
 }
\$\endgroup\$
6
\$\begingroup\$

Your consumer thread locks out the producer for extended periods of time.

void ConsumeData()
 {
   while (true)
   {
    std::unique_lock<std::mutex> uniqueLock(mutex);
    conditionVariable.wait(uniqueLock, [] {return !queue.empty(); });
    while (!queue.empty())
    {
        // DO WORK
        queue.pop();
    }
    uniqueLock.unlock();
   }
}

I would rather see the consumer grab a single item from the queue then release the queue (so the producer can continue to add work while the consumer is processing a single job.

void ConsumeData()
{
    while (true)
    {
      {
        std::unique_lock<std::mutex> uniqueLock(mutex);
        conditionVariable.wait(uniqueLock, [] {return !queue.empty(); });

        // Note: Don't need to test queue.empty()
        //       If it is empty then the thread is re-queued on the
        //       condition variable it is only released when the test
        //       above is true.
        work = queue.front();
        queue.pop();
      }
      // You have the work item so you don't need the lock.
      // So out here you work on the item you just retrieved from
      // queue. When done you loop back around to get more work.

      // DO WORK
    }
  }
}

Your producer adds lots of items to the queue. But only signals once. You should signal once for every item added to the queue.

  for (std::uint8_t count = 0; (count < maxWordCount) || (count != '\0'); ++count) {
    queue.push(word);
    conditionVariable.notify_one();
  }

As a side note:

An indent of 2 is a bit on the low side (acceptable by some). But in my opinion makes the code too cluttered.

\$\endgroup\$
  • \$\begingroup\$ I noticed that indent only by two spaces is trending amongst Mac users. Usually they also have micro font size. \$\endgroup\$ – Incomputable Feb 14 '18 at 18:09
  • \$\begingroup\$ @Incomputable The LLVM style guide in particular recommends usage of 2 spaces, which could be part of it. \$\endgroup\$ – JAB Feb 14 '18 at 22:02
  • \$\begingroup\$ @Incomputable: The trouble is this is one of those things you can't really argue against. It is totally opinion based there is no right/wrong answer. You just have to decide on an arbitrary value (but it should be consistent through the team, so the rest of the team should be using the same style, so it is on of those things that go im the style guide). \$\endgroup\$ – Martin York Feb 14 '18 at 23:11
  • \$\begingroup\$ @MartinYork, I believe clang-format solves the problem, the created problem being that not everybody knows how to use it. I encounter it sometimes on Github C++ projects, but even then it is not omnipresent. \$\endgroup\$ – Incomputable Feb 15 '18 at 15:11
3
\$\begingroup\$

I see a number of things that may help you improve your program.

Isolate platform-specific code

If you must have stdafx.h, consider wrapping it so that the code is portable:

#ifdef WINDOWS
#include "stdafx.h"
#endif

Make sure you have all required #includes

The code uses std::thread but doesn't #include <thread>. It was not difficult to infer, but maintenance is simplified if all of the required #includes are present. While the code may compile on your machine as is, if it does it is implicitly relying on one of the other named headers to include the missing one. This is not portable and eventually becomes a maintenance headache. Best is to use the documented #includes from the beginning.

Use only required #includes

Only include files that are actually needed. Nothing from the <sstream> header appears to be needed here, so I'd recommend omitting it.

Supply the missing isVowel function

This is more a matter of helping reviewers rather than necessarily a flaw in your code, but without the implementation of the isVowel code, reviewers are forced to guess what that code does.

Allow the user to specify input file name

The input file name is currently hardcoded which certainly greatly restricts the usefulness of the program. Consider using argc and argv to allow the user to specify file names on the command line.

Avoid data races

The ProduceData and ConsumeData functions both potentially access std::cout but that's a single resource that might simultaneously be used by other threads. One fix for this is to use a std::mutex like this:

std::mutex cout_mutex;

// wherever cout is used:
some_function() {
    std::lock_guard<std::mutex> lock(cout_mutex);
    std::cout << "Now we can do this safely!\n";
}

Note that std::lock_guard is intended to be used via RAII so that the lock is obtained when the object is created and released when it's destroyed, making it easy to avoid the bug of forgetting to unlock the mutex.

Avoid the use of global variables

The current code has mutex, queue and conditionVariable as global variables. It's generally better to explicitly pass variables your function will need or declare them within the appropriately smallest possible scope rather than using the vague implicit linkage of a global variable. Also, those are very generic names that are not very helpful to understanding what the code is doing. For example, I generally prefer to name a mutex after the thing it's protecting to make it easier to see why it's being used at all; for that reason, in this case I'd suggest q_mutex or the like.

Minimize the time a lock is held

The code currently contains these lines:

while (!queue.empty())
{
    const std::string& str = queue.front();
    std::size_t numVowels = std::count_if(str.begin(), str.end(), isVowel);
    std::cout << str << "-" << numVowels;
    queue.pop();
}
uniqueLock.unlock();

This effectively keeps the queue locked until the queue is entirely emptied and all of the output has been written. That's potentially a long time and also not really necessary. It's good design to try to always minimize the time a lock is held. In this case I'd probably just go through once rather than exhausting the queue, and do the printing outside the protection of the lock. If string duplication is fast enough, one could rewrite it like this:

std::string str;
if (!queue.empty())
{
    str = queue.front();
    queue.pop();
}
uniqueLock.unlock();
std::size_t numVowels{std::count_if(str.begin(), str.end(), isVowel)};
std::cout << str << "-" << numVowels;

Better, of course, would be to take the advice above and also obtain a lock for std::cout.

Fix the bug

The producer currently has this curious bit of code

for (std::uint8_t count = 0; (count < maxWordCount) || (count != '\0'); ++count)
    queue.push(word);

The problem with that is that even for a count >= maxWordCount the second part of the || clause will be true, leading to an endless loop. Better would be to simply push a word at a time and simply use queue.push(word); without the loop.

\$\endgroup\$
  • \$\begingroup\$ std::cout is thread-safe guaranteed by the C++ standard, so no need to wrap it with a muted. If there's logically related output that's separated into multiple streaming operations, it's much nicer regarding multithreading to use a temporary stringstream \$\endgroup\$ – stefan Feb 14 '18 at 22:47
  • \$\begingroup\$ More information on thread-safety of std::cout stackoverflow.com/a/15034536/985296 \$\endgroup\$ – stefan Feb 14 '18 at 22:57
  • 2
    \$\begingroup\$ Technically correct, but the effect is that one can end up with interleaved output as the different threads write asynchronously. \$\endgroup\$ – Edward Feb 15 '18 at 2:06
  • \$\begingroup\$ @stefan, not only atomicity, but order matters as well. The latter is solved in C++20 \$\endgroup\$ – Incomputable Feb 15 '18 at 5:32
  • \$\begingroup\$ @Edward exactly, but this can be solved without introducing locks, as shown in the linked answer \$\endgroup\$ – stefan Feb 15 '18 at 6:26

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.