-1

I'm trying to implement a non-blocking serial communication in my C++ app. A thread is responsible to do serial communication, and I've written a ThreadSafeClass to exchange data between serial thread and main thread. Here is the core of my code:

main.cpp

#include "serial.hpp"
#include "tsqueue.hpp"

int main(int argc, char *argv[])
{
    serial::init();
    while (true)
    {
        fgets(s);
        serial::outQueue.enqueue(std::string(s));
    }
    serial::shutdown();
    return 0;
}

tsqueue.hpp

#include <mutex>
#include <queue>

namespace tsqueue
{

template <typename T>
class ThreadSafeQueue
{
  private:
    mutable std::mutex _mtx;
    std::queue<T> _que;

  public:
    ThreadSafeQueue();
    ~ThreadSafeQueue();
    void enqueue(const T &item);
    T tryDequeue(const T &defaultValue, bool &done);
    void clear();
    bool isEmpty() const;
};

template <typename T>
ThreadSafeQueue<T>::ThreadSafeQueue() {}

template <typename T>
ThreadSafeQueue<T>::~ThreadSafeQueue() { clear(); }

template <typename T>
void tsqueue::ThreadSafeQueue<T>::enqueue(const T &item)
{
    std::lock_guard<std::mutex> lock(_mtx);
    _que.push(item);
}

template <typename T>
T tsqueue::ThreadSafeQueue<T>::tryDequeue(const T &defaultValue, bool &done)
{
    std::lock_guard<std::mutex> lock(_mtx);
    if (_que.empty())
    {
        done = false;
        return defaultValue;
    }
    else
    {
        T item = _que.front();
        _que.pop();
        done = true;
        return item;
    }
}

} // namespace tsqueue

And serial declaration/definition,

serial.hpp

#include <string>
#include "tsqueue.hpp"

namespace serial
{

static tsqueue::ThreadSafeQueue<std::string> inQueue;
static tsqueue::ThreadSafeQueue<std::string> outQueue;

void init();
void shutdown();

}

serial.cpp

#include <string>    
#include "serial.hpp"
#include "tsqueue.hpp"

static std::thread _thread;

void run()
{
    while (true)
    {
        std::string str = serial::outQueue.tryDequeue(emptyStr, dequeued);
        if (dequeued) { /* Do send 'str' */ }
        if (terminationRequested) { break; }
        // Some sleep
    }
}

void serial::init()
{
    serial::inQueue.clear();
    serial::outQueue.clear();
    _thread = std::thread(run);
}

void serial::shutdown()
{
    if (_thread.joinable()) { _thread.join(); }
}

The problem is, when tryDequeue(...) is called by serial thread's run() in serial.cpp, it always sees empty outQueue. However while loop still sees outQueue in main.cpp with provided data, even at later times. I've find out that using debug tools of vscode. I'm new to C++, but experienced in other languages. What am I doing wrong in above code? Do run() and main() see different objects?

Compiler: g++ 7.3.0, Environment: Linux (Ubuntu 18.04)

Edit: If I remove static from definitions of inQueue and outQueue, I get multiple definition error by linker for both. Although I have appropriate include guards.

h.nodehi
  • 1,036
  • 1
  • 17
  • 32
  • Is this [mcve]? If so, you have a race condition. You expect the main thread to add an object to the queue, before the worker thread checks for existence, without doing anything to guarantee such order. Everything can just work in the following order instead: worker thread is started, worker thread tries to dequeue (failing to find any element in the queue), exits, main thread adds an element to the queue. – Algirdas Preidžius Feb 12 '19 at 21:47
  • @AlgirdasPreidžius there is no race condition, as per definition of the term. Rather, there is an unpredictable program outcome, but not every unpredictable outcome is a race condition. – SergeyA Feb 12 '19 at 21:50
  • 1
    Do not manually lock mutexes, instead use `std::lock_guard` and `std::unique_lock`. – SergeyA Feb 12 '19 at 21:51
  • 1
    Recommendation: [Look into `std::lock_guard`](https://en.cppreference.com/w/cpp/thread/lock_guard) It is a really nifty [RAII-based](https://stackoverflow.com/questions/2321511/what-is-meant-by-resource-acquisition-is-initialization-raii) tool to make sure that the mutex is unlocked in the event of errors. – user4581301 Feb 12 '19 at 21:52
  • @AlgirdasPreidžius code is edited now. It has a while loop. The code in run() doesn't run just once. – h.nodehi Feb 12 '19 at 21:58
  • @SergeyA , @user4581301 , wouldn't be a high cost in instantiating `lock_guard` so many times in each function call? – h.nodehi Feb 12 '19 at 22:02
  • 2
    The cost would be the same as with simply calling `mutex.lock()`. If you are fine with this call, you should be fine with `lock_guard.lock()`. – SergeyA Feb 12 '19 at 22:05
  • 1
    See answer below on how to properly remove the static. You need extern in the header and no static in the cpp file. Then it will work. Of course it would be better to avoid global variables. – Johannes Overmann Feb 12 '19 at 23:09
  • @JohannesOvermann How to avoid global variables? Maybe references by functions? – h.nodehi Feb 12 '19 at 23:19
  • @SergeyA "_there is no race condition, as per definition of the term._" What is the definition of the term, then? Since the one, that I could find is: "A race condition or race hazard is the behavior of an electronics, software, or other system where the system's substantive behavior is dependent **on the sequence or timing of other uncontrollable events**" And this (was) precisely the problem: If instructions were sequenced differently, than one expected, the system's behavior worked unexpectedly. – Algirdas Preidžius Feb 13 '19 at 11:42
  • @AlgirdasPreidžius race condition could be a wider defined term that my comment implied, but in C++, race condition is usually associated with undefined behavior. There is no undefined behavior in provided example, this is what I meant to say. – SergeyA Feb 13 '19 at 14:34

1 Answers1

2

(Heavily edited after all the non-issues have been repaired and after I finally spotted what was the actual problem:)

The problem is that you have two completely separate instances of outQueue: One in main.o and one in serial.o (or .obj if you are on Windows). The problem is that you declare these as static in a header. That results in individual copies of this in every *.cpp/object which included this header.

Ideally outQueue would not be a global variable. Assuming it should be a global variable you can fix this like this:

serial.hpp

namespace serial
{    
    // This is a declaration of a global variable. It is harmless 
    // to include this everywhere.
    extern tsqueue::ThreadSafeQueue<std::string> inQueue;
    extern tsqueue::ThreadSafeQueue<std::string> outQueue;        
}

serial.cpp

namespace serial
{    
    // This is the actual definition of the variables. 
    // Without this you get unresolved symbols during link time
    // (but no error during compile time). If you have this in 
    // two *.cpp files you will get multiple definition linker 
    // errors (but no error at compile time). This must not be 
    // static because we want all other objects to see this as well.
    tsqueue::ThreadSafeQueue<std::string> inQueue;
    tsqueue::ThreadSafeQueue<std::string> outQueue;        
}

The ThreadSafeQueue itself looks ok to me.

Johannes Overmann
  • 4,914
  • 22
  • 38
  • @SergeyA: Please help me: Why can't i pass a const reference into `enqueue()`? I was thinking along the lines of `enqueue(const std::string& item)` for a concrete instantiation of the template. – Johannes Overmann Feb 12 '19 at 21:53
  • Code is edited now. It actually has has an infinite while loop. – h.nodehi Feb 12 '19 at 21:54
  • @JohannesOvermann, sorry, my bad. I read `dequeue` instead of `enqueue`. – SergeyA Feb 12 '19 at 21:56
  • @h.nodehi: You are enqueueing into outQueue and dequeueing from myQueue which is not defined. Do you mean inQueue or outQueue? – Johannes Overmann Feb 12 '19 at 21:57
  • @SergeyA: No problem. And I am already reading outQueue instead of myQueue. :-) I did not spot that at first! It may be the OP is accidentally read from the wrong queue. I make mistakes like that. In your own code you don't see that. – Johannes Overmann Feb 12 '19 at 21:59
  • @JohannesOvermann sorry, that was a mistype. Obviously it is `outQueue`. – h.nodehi Feb 12 '19 at 22:06
  • 1
    I have it! You have two completely separate instances of `outQueue`: One in main.o and one in serial.o! This must be it and explains the behavior you see. Make the outQueue really global and reference it in the other module. – Johannes Overmann Feb 12 '19 at 22:55
  • @JohannesOvermann thanks. That worked! As I said, I'm new to C++. What is the problem? Why two instances even when include guards are provided? – h.nodehi Feb 12 '19 at 23:18
  • 1
    @h.noedi: Your build process has two stages: Compiling and then linking. Each *.cpp file is first compiled separately, and each *.cpp file includes the header file once. The include guard just makes sure it is just included once _per *.cpp file_. So your static variables end up in two compilation units and you get either wrong behavior (w. static) or when you simply remove the static you get multiple definitions of the same thing. The latter is a hint that with static you get also multiple instances, but they do not conflict in the link stage because their symbols are not visible to t. linker. – Johannes Overmann Feb 12 '19 at 23:35