Sharing data between two threads C++

Question

I'm working with a multithreaded project where the main thread runs the ui/gui part, and the process_thread will contain data received from an industrial plc via an external library, which can be time-critical, but maybe slow, blocking realtime data reading.

Now I'm just simulating this very simple in the Process class and the gui is here simplified with just printing the simulated/generated values to console.

Since the data will be read only from the process_thread, maybe some flags will be set back to the process thread via the class to change settings etc. I choosed to detach the thread and share the data via the Process class, without the use of any mutex/lock syncing.

Is this way of sharing data and running two threads OK in this context?
Other comments of issues or improvements?

process.h

#pragma once
#include <vector>
#include <chrono>

namespace app{

    class Process_sim {
        using ms = std::chrono::milliseconds;
    public:
        Process_sim(){}
        void run();
        inline std::vector<double>& data() { return m_vec; }
        inline ms cycle_interval() { return m_cycle_time; }
        inline void set_cycle_interval(int millis) { m_cycle_time = static_cast<ms>(millis); }
        inline void kill() { m_run = false; }
        inline bool is_alive() const { return m_run; }

    private:
        std::vector<double> m_vec;
        std::vector<double> m_default_vec;
        bool m_run = true;
        static constexpr int m_num_elem {5U};
        double m_sim_factor {1.235}; // initialize with some random nr
        static constexpr double max_val {500.0};
        ms m_cycle_time = static_cast<ms>(200); // start with 200ms

    };
}

process.cpp

#include "stdafx.h"
#include "Process.h"

namespace app {

    void Process_sim::run()
    {
        using time = std::chrono::high_resolution_clock;
        auto t0 = time::now();

        for (int i = 0; i < m_num_elem; ++i) m_vec.push_back(m_sim_factor + i);

        m_default_vec = m_vec;

        t0 = time::now() + m_cycle_time;

        while (m_run) {
            if (time::now() >= t0)
            {
                for (int i = 0; i < m_vec.size(); ++i) {
                    m_vec[i] += m_sim_factor + i;

                    //If the last element is above max, reset to default
                    if (i == m_vec.size()-1 && m_vec[i] > max_val) m_vec = m_default_vec;
                }
                t0 = time::now() + m_cycle_time; //set the new future time
            }
        }
    }
}

console.h

#include "Process.h"
#include <vector>
#include <chrono>

namespace app {

    class Console {
        using sec = std::chrono::seconds;
        //using ms = std::chrono::milliseconds;
    public:
        Console(Process_sim*);
        void run();

    private:
        Process_sim* m_process;
        std::vector<double> m_data;
        //int m_frames_sec = 20;
        sec m_cycle_time = static_cast<sec>(1); // print once a second
    };
}

console.cpp

#include "stdafx.h"
#include "console.h"
#include <string>
#include <iostream>
#include <iomanip>

namespace app {

    Console::Console(Process_sim* p)
        : m_process{p} 
    {}
    //----------------------------------------------------------------------------------------------
    using time = std::chrono::high_resolution_clock;
    auto t0 = time::now();

    void Console::run()
    {
        int loop_count = 0;
        static constexpr int num_loops = 20U;

        t0 = time::now() + m_cycle_time;

        //Run until we decide to kill process thread
        while (m_process->is_alive() )
        {
            if (time::now() >= t0)
            {
                m_data.clear();
                m_data = m_process->data();

                if (m_data.size() && loop_count == 0) std::cout << "Data from process thread, vector size:"
                                        << m_data.size() << "\n\n";

                for (auto it : m_data) std::cout << std::fixed << std::setprecision(2) << it << " - ";
                std::cout << "\nRemaining cycles: " << num_loops - loop_count-1 << "\n\n";
                t0 = time::now() + m_cycle_time;

                if (++loop_count == num_loops) m_process->kill();
            }           
        }
    }
}

main.cpp

#include "stdafx.h"
#include <iostream>
#include <thread>
#include "Process.h"
#include "console.h"

void t1(app::Process_sim& p)
{
    p.run();
}


int main() 
{
    using namespace app;


    Process_sim* process_sim = new Process_sim;  //processes common data to be shared with main thread(read only).
    Console console(process_sim);

    std::thread process_thread(t1, std::ref(*process_sim));

    if (process_thread.joinable()) process_thread.detach(); 

    console.run();

    delete process_sim;

    char ch;
    std::cout << "\n\nEnter a key to exit";
    std::cin >> ch;

    return 0;
}

Answer 1

The short answer is no, this is not thread-safe. Detaching the thread has no effect on the thread safety.

There are multiple ways to make this thread safe. As you said that the producer is slow running and might block, one way would be to buffer the output of the producer once it's done and then return the buffered result to the consumer (your UI) when it request it. If the data is large, the process could provide a change count so that the UI can check that there is actually new data before calling data(). The copy operation and the access to the copied data should still be secured via a mutex. For example:

Process_sim::run() {
   ...
   // processing is finished
   backup_data();
}

void Process_sim::backup_data() {
  std::lock_guard<std::mutex> lock(m_data_mutex);
  m_backup_data = m_data; 
}

std::vector<double> Process_sim::data() {
  std::lock_guard<std::mutex> lock(m_data_mutex);
  return m_backup_data;
}

std::lock_guard and std::mutex are C++11 constructs; if you don't have those you would have to check what is provided by your system.

You don't want to return a reference to your data in data(). This would break thread safety if the caller actually takes the reference and uses it while the object is being modified.

In your process you are using busy wait. This might be appropriate in the device where the process code is running; in modern C++ you would want to use std::sleep_for or std::sleep_until or similar.