Make intra-process manager thread safe, rename IPMState to IPMImpl #165
Conversation
|
Whoops, I needed to rename the class in a few other places. http://ci.ros2.org/job/ci_linux/653/ |
| @@ -184,6 +186,7 @@ class IntraProcessManagerState : public IntraProcessManagerStateBase | |||
| size_t & size | |||
| ) | |||
| { | |||
| std::lock_guard<std::mutex> lock(runtime_mutex_); | |||
There was a problem hiding this comment.
Can this lock be moved into the next block, where we iterate over publishers_? Or do we need to hold it all the way through the other two blocks?
There was a problem hiding this comment.
To implement this more optimally, I could have one mutex for publishers_ and then a mutex for each PublisherInfo entry in publishers_. That way one thread could look up an entry in publishers_ while the other is looking up something in the map owned by an entry in publishers (I believe that would be fine). And yes, that implementation would include moving the mutex for publishers_ into the block where find is invoked one the map.
There was a problem hiding this comment.
A mutex per item sounds like overkill; I would leave it as is.
I hadn't dug into the data types being handled, and didn't realize that you still need exclusion after pulling the items out of publishers_.
|
Without this change, I pretty reliably get a segfault after a dozen or so iterations of the test added in ros2/system_tests#72. With this change, I'm now at > 100 iterations with no problem. FYI, the segfault (from a Debug build of ROS 2, but with a non-Debug build of opensplice) starts like this: |
|
I can run up to iteration 650 of the multithreaded test and then I get an error from OpenSplice: I get a similar error when I repeat single-threaded tests indefinitely, however, so I don't think it's relevant. |
|
@jacquelinekay I get the same OpenSplice error, in my case after 116 iterations. Agreed that it's not related to this change (though it may indicate a resource management issue that we'll have to tackle at some point). |
|
+1 |
a29cc96 to
67151de
Compare
Make intra-process manager thread safe, rename IPMState to IPMImpl
|
Please create a ticket for the resource problem to keep track of it. |
* add timer test * more tests * another one just for fun * uncrustify
Generate rclcpp::Node before start_recording since rclcpp::Node will set parameters of use_sim_time and publish message to parameter_events. This will cause the wrong messages count in the test. Signed-off-by: evshary <evshary@gmail.com>
To implement a lock-free IntraProcessManagerImpl in the future, I will extend from IntraProcessManagerImplBase and use lock-free structures instead of mutexes.