0

I have a class:

class IOWorker {

std::thread                                    thread_;
boost::asio::io_service                        ios_;
boost::optional<boost::asio::io_service::work> work_;
Callback                                       callback_;

// Map of thread::id and this class' instance
typedef std::map<std::thread::id, IOWorker *>  IOWorkerThreadMap;
static IOWorkerThreadMap                       iOWorkerThreadMap;

public:
    IOWorker(Callback cb);
    ~IOWorker();
    
    std::thread::id getThreadId() {
       return thread_.get_id();
    }
    
    // IO worker threads will call this to fetch their instance
    static IOWorker* getIOWorkerInstance (void) {
       auto it = iOWorkerThreadMap.find(std::this_thread::get_id());
       if (it == iOWorkerThreadMap.end()) {
           return nullptr;
       }
       return (it->second);
    }
};

IOWorker::IOWorker (Callback cb) : callback_{cb}
{
    work_ = boost::in_place(boost::ref(ios_));
    thread_ = std::thread{[this] () {
               ios_.run();
                  }
           };
}

In a function executed by the main thread, I'm creating 10 instances of this class and inserting these into the map where thread::id is key and class instance is value. I'm accessing this map from all the worker threads to fetch their respective class instance by looking up their thread::id in the map. The main thread accesses these instances too, to call some methods, post jobs on ios_, etc.

void startIOWorkers (Callback cb)
{
   for (int i = 0; i < 10; ++i) {
       IOWorker *iow = new IOWorker{cb};
       std::thread::id threadId = iow->getThreadId();
       IOWorkerThreadMap.insert(std::make_pair(threadId, iow));
   }
}

My question is for below line:

IOWorkerThreadMap.insert(std::make_pair(threadId, iow));

My understanding (could be wrong!) is that iow and threadId in above function, will be "copied" while I'm inserting them in the map and two copies of them will exist.

I want to avoid that, therefore, I would like to know what are the better ways of having a map of thread::id and class instance in this case?

void
  • 338
  • 5
  • 19
  • 1
    Am I missing something, or wouldn't just using a `thread_local IOWorker* this_thread_worker;` be a lot simpler? –  Jul 30 '21 at 19:52
  • Your code shows no synchronization to ensure that the static map is not being accessed by two or more threads simultaneously. – PaulMcKenzie Jul 30 '21 at 19:55
  • @PaulMcKenzie, It is taken care in original code. Removed it to reduce the clutter in "example". Even the class definition is shortened to a great extent. – void Jul 30 '21 at 19:58
  • Then I'm not seeing why creating the map entry would be a concern. If everything is synchronized, I don't see any issues with what you're claiming could be an issue. So what if a copy is made (that's how value-based containers work). – PaulMcKenzie Jul 30 '21 at 20:01
  • @PaulMcKenzie, https://stackoverflow.com/questions/5687386/when-a-key-value-is-inserted-into-a-stdmap-does-it-make-its-own-copy-of-t By going through this thread, I learned that inserting entry in any std container is a copy. Hence, my concern of having `iow` and `threadId` local to the function and inserting them into the map will result in two copies. – void Jul 30 '21 at 20:10
  • 1
    Of course there will be two copies. But what's the issue if the local version will go out of scope anyway? You might as well ask this for any C++ code that inserts values in a map, vector, std::list, etc. – PaulMcKenzie Jul 30 '21 at 20:11
  • No issues actually, I was just thinking whether it is a good practice to do that or better to "move" the local version to the map. – void Jul 30 '21 at 20:15
  • In your code, `iow` is a pointer. Why would you care how many copies of the same pointer there are? – David Schwartz Jul 30 '21 at 21:08

1 Answers1

4

This seems way more complicated than it has to be.

If, as it appears to be the case, you only need to access the map at from thread::this_thread, the language already has a map<thread_id, T> built right in: thread_local.

class IOWorker {

std::thread                                    thread_;
boost::asio::io_service                        ios_;
boost::optional<boost::asio::io_service::work> work_;
Callback                                       callback_;

static thread_local IOWorker*                  this_thread_worker_;

public:
    IOWorker(Callback cb);
    ~IOWorker();
    
    // IO worker threads will call this to fetch their instance
    static IOWorker* getIOWorkerInstance (void) {
       return this_thread_worker_;
    }
};

IOWorker::IOWorker (Callback cb) : callback_{std::move(cb)}
{
    work_ = boost::in_place(boost::ref(ios_));
    thread_ = std::thread{[this] () {
       this_thread_worker_ = this;
       ios_.run();
    };
}


std::vector<IOWorker> startIOWorkers(Callback cb)
{
    std::vector<IOWorker> launched_workers;
    launched_workers.reserve(10);
    
    for (int i = 0; i < 10; ++i) {
       launched_workers.emplace_back(cb);
    }

    return launched_workers;
}
  • Actually, in one case, the main thread also accesses these instances to post some work for the threads to do. And hence I thought of maintaining the aforementioned map. – void Jul 30 '21 at 20:18
  • So yeah, the main thread would also like to access these instances, call some methods, post jobs on ios_, etc. How can we achieve that using your solution? – void Jul 30 '21 at 20:20
  • @void The main thread can just keep a container of pointers to the Workers. The main thread having a list of workers, and the worker threads being able to get a pointer back to their respective `IOWorker` objects are unrelated features and can be handled separately. –  Jul 30 '21 at 20:45
  • 1
    @void I've updated the answer with an example of how the main thread could keep track of the launched workers. Something needs to manage the lifetime of these objects anyways, so you would have had to do that at some point anyways, –  Jul 30 '21 at 20:50
  • emplace_back is new to me. In my example, when I have to shutdown these worker threads from main thread, I used to iterate the map and for each `new` that I did earlier, I would do `delete it.second` that would translate to `delete IOWorker*` which would invoke the destructor that would do `ios_.stop(); thread_.join();`. I'm not quite sure how can I do that with emplace_back with a vector. – void Jul 30 '21 at 21:02
  • 2
    @void When the vector is destroyed, the IOWorkers will be destroyed alongside it. It's literally that simple. Just keep the vector around until it's time to destroy the workers. There are incredibly few reasons to ever use new/delete anymore. –  Jul 30 '21 at 21:04
  • Thanks for the clarification. I tried the approach mentioned by you but getting the error `"error: call to implicitly-deleted copy constructor of 'IOWorker'"` `::new((void*)__p) _Up(_VSTD::forward<_Args>(__args)...); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~` with above changes and class. – void Aug 04 '21 at 12:58