Transition from boost::thread to std::thread. #3060
Conversation
SergioRAgostinho
added
needs: code review
|
If possible, give priority to this one. I have some other PRs pending on it. |
| @@ -566,7 +566,7 @@ pcl::HDLGrabber::stop () | |||
| bool | |||
| pcl::HDLGrabber::isRunning () const | |||
| { | |||
| return (!hdl_data_.isEmpty () || (hdl_read_packet_thread_ != nullptr && !hdl_read_packet_thread_->timed_join (boost::posix_time::milliseconds (10)))); | |||
| return (!hdl_data_.isEmpty () || hdl_read_packet_thread_); | |||
There was a problem hiding this comment.
Could you help me to understand what was the point of timed_join and why we don't need it anymore? (I do know that std::thread does not have it, but still.)
There was a problem hiding this comment.
Ha, indeed we're missing some logic if we just remove everything.
The underlying logic with timed_join was the following: you request the thread to join under a specified time and if it manages to successfully do it, it returns true, otherwise false. If your grabber is running normally, there´s no chance for the thread to join, hence the check for that condition. As you said, we no longer have access to timed_join, but usually all these grabbers have an exit flag - terminate_read_packet_thread_ in this case - which marks the intent to break out of the grabber loop. This exit flag is not a real/pure proxy to check if the grabber is still running or not, but it gives you an indication of what will happen very soon, making it a potentially viable replacement candidate to check. Ultimately, I can have a look on how to fully replace this functionality with a condition_variable. It might actually not be super involved.
There was a problem hiding this comment.
Thanks. With this PR there will be a brief period between stop() is called and the thread has joined during which isRunning() will still return true. I don't think this is a problem. Technically, we are still grabbing if the thread hasn't joined.
There was a problem hiding this comment.
With this PR there will be a brief period between
stop()is called and the thread has joined during whichisRunning()will still returntrue.
To prevent that we can add a check to terminate_read_packet_thread_. This is the first variable to get set to true once stop() is invoked. I see no harm in doing it.
There was a problem hiding this comment.
Yep, but setting that flag does not terminate the grabber thread right away. It may happen that one more frame is read before it has an effect.
Ultimately this comes down to the fact that we don't have a strict definition what "is running" means. Is it when the thread is still active? Is it until stop() is called? But I guess it's not that important. If you prefer, you can add the check you mentioned, then we will adopt the second definition.
There was a problem hiding this comment.
No preference. Since there's no strict definition and currently no reason to create one I say we could leave as it is.
Just dumping this here to see if it compiles and if it looks ok for review.