Factory for threads in C-CodePudding

I am currently trying to create a sort of "factory" pattern in a class, whose instances should be Threads that have their own certain operation procedure. I have currently declared a global variable isFinished in order to end the operation of the worker thread, however, somehow the operation does not stop after the variable state is changed, and the worker thread does not do the std::thread::join() operation in order to finish working. I am new to threads, and in C in general and I don't really know what is wrong so any help would be greatly appreciated.

Here is the code:

\\main.cpp
#include <iostream>
#include <thread>
#include <atomic> 
#include "Sender.h"

int main()
{
    isFinished = false; //set false for worker thread
    Sender mSender; //Create thread object for worker
    std::cin.get(); //Trigger by pressing enter
    isFinished = true; //exit worker while loop
    mSender.join(); //wait for the worker thread to finish execution
}

\\sender.cpp
#include <iostream>
#include <thread>
#include <atomic> 
#include "Sender.h"

void Sender::SenderWorkflow()
{
    while (!isFinished)
    {
        std::cout << "Working... \n";
    }
}

\\sender.h
#include <iostream>
#include <thread>
#include <atomic> 

static std::atomic<bool> isFinished;

class Sender {
public:
    std::thread worker;

    Sender()
        : worker(&Sender::SenderWorkflow, this)
    {
    }

    void SenderWorkflow();

    void join() {
        worker.join(); \\std::thread::join()
    }
};

CodePudding user response：

The problem is that you are using a static variable in the global scope. static in a namespace scope is used to hide the variable or function in the object file so that the linker can't see it while linking another object file for another cpp file. In practice static is used only inside the cpp file itself. Since you declared it inside the header each translation unit that includes it will get a different instance of the variable because the linker can't see the same variable across objects file. To solve this :

1- move the declaration of the static variable to the cpp file and make an accessor function in the header file:

\\sender.h
#include <atomic> 

const std::atomic<bool>& checkIsFinished();
void setFinished();

\\sender.cpp
static std::atomic<bool> isFinished;

const std::atomic<bool>& checkIsFinished() { return isFinished; }
void setFinished() { isFinished = true; }

2- or use inline variable (since c 17) in the header file:

\\sender.h
#include <atomic> 

inline std::atomic<bool> isFinished;

3 - or better and the correct design : make the variable local as member of the class, the less global variables the better !

\\sender.h
#include <atomic>

class Sender {
public:
    std::thread worker;

    Sender()
        : worker(&Sender::SenderWorkflow, this)
    {
    }

    void SenderWorkflow();

    void join() {
        isFinished = true;
        worker.join(); \\std::thread::join()
    }
private:
    std::atomic<bool> isFinished = false;
};