Thread synchronization problem in thread's procedure

Question

I have a question. I add the object to the map and in the thread call the run() procedure for all elements in the map. I correctly understand that in this code there is a synchronization problem in the process procedure. Can I add a mutex? Given that this procedure is called in the thread?

class Network {

public:
  Network() {
    std::cout << "Network constructor" << std::endl;
  }

  void NetworkInit(const std::string& par1) {
    this->par1 = par1;
  }

  ~Network() {
    std::cout << "Network destructor" << std::endl;
    my_map.clear();
  }

  void addLogic(uint32_t Id, std::shared_ptr<Logic> lgc) {
    std::lock_guard<std::mutex> lk(mutex);
    my_map.insert(std::pair<uint32_t, std::shared_ptr<Logic>>(Id, lgc));
    cv.notify_one();
  }

  void removeLogic(uint32_t Id) {
    std::unique_lock<std::mutex> lk(mutex);
    cv.wait(lk, [this]{return !my_map.empty(); });

    auto p = this->my_map.find(roomId);
    if (p != end(this->my_map)) {
      this->my_map.erase(roomId);
    }

    lk.unlock();
  }

 /**
   * Start thread
   */
  void StartThread(int id = 1) {
    running = true;
    first = std::thread([this, id] { process(id); });
    first.detach();
  }

  /**
   * Stop thread
   */
  void StopThread() {
    running = false;
  }

private:
  std::thread first;
  std::atomic<bool> running = ATOMIC_VAR_INIT(true);

  void process(int id) {
    while (running) {
      for (const auto& it:my_map) {
        it.second->run();
      }
      std::this_thread::sleep_for(10ms);
    }
  }    

 private:
  std::mutex mutex;
  std::condition_variable cv;

  using MyMapType = std::map<uint32_t, std::shared_ptr<Logic> >; 

  MyMapType my_map;
  std::string par1;
};

Answer 1

The solution with low level concurrency primitives usually does not scale and is not easy to maintain.

A better alternative would be to have a thread-safe "control" queue of map update or worker termination instructions.

Something like this:

enum Op {
        ADD,
        DROP,
        STOP
};

struct Request {
    Op op;
    uint32_t id;
    std::function<void()> action;
};

...

// the map which required protection in your code
std::map<uint32_t, std::function<void()>> subs;

// requests queue and its mutex (not very optimal, just to demonstrate the idea)
std::vector<Request> requests;
std::mutex mutex;

// the worker thread
std::thread worker([&](){
    // the temporary buffer where requests are drained to from the queue before processing
    decltype(requests) buffer;

    // the main loop
    while (true) {
        // requests collection (requires synchronization)
        {
            std::lock_guard<decltype(mutex)> const guard {mutex};
            buffer.swap(requests);
        }

        // requests processing
        for(auto&& request: buffer) {
            switch (request.op) {
                case ADD:
                    subs[request.id] = std::move(request.action);
                    break;
                case DROP:
                    subs.erase(request.id);
                    break;
                case STOP: goto endloop;
            }
        }

        // map iteration
        for (auto&& entry: subs) {
            entry.second();
        }
    }
    endloop:;
});

Answer 2

The first idea is to protect the map as a whole with a mutex that is released during run. This works for addLogic because inserting into a map invalidates no iterators, but not for deleteLogic which might invalidate the very iterator value being used by process.

More efficient, lock-free approaches like hazard pointers may be applicable here, but the basic idea is to use a deferred deletion list. Assuming that the intent of concurrent deletion is cancellation of the task (not merely cleanup after all work is completed), it’s sensible to have the consumer thread to check immediately before execution. Using a set (to correspond to your map) will let the deletion list be dynamic and those checks be efficient.

So have another mutex protect the deletion list and take it at the beginning of each iteration in process:

  void addLogic(uint32_t Id, std::shared_ptr<Logic> lgc) {
    std::lock_guard<std::mutex> lk(mutex);
    my_map.insert(std::pair<uint32_t, std::shared_ptr<Logic>>(Id, lgc));
  }

  void removeLogic(uint32_t Id) {
    std::lock_guard<std::mutex> kg(kill_mutex);
    kill.insert(Id);
  }

private:
  std::set<uint32_t> kill;
  std::mutex mutex,kill_mutex;
  void process(int id) {
    for(;running;std::this_thread::sleep_for(10ms)) {
      std::unique_lock<std::mutex> lg(mutex);
      for(auto i=my_map.begin(),e=my_map.end();i!=e;) {
        if(std::lock_guard<std::mutex>(kill_mutex),kill.erase(i->first)) {
          i=my_map.erase(i);
          continue;    // test i!=e again
        }
        lg.unlock();
        i->second->run();
        lg.lock();
        ++i;
      }
    }
  }

This code omits your condition_variable usage: it’s not necessary to wait before enqueuing something for deletion.