invalid_argument when using intra-process comms with queue size 0

[cottsay]

Bug report

Required Info:

• Operating System:
  • Ubuntu 16.04
• Installation type:
  • source
• Version or commit hash:
  • 005131
• DDS implementation:
  • Fast-RTPS
• Client library (if applicable):
  • rclcpp

Steps to reproduce issue

1. Set use_intra_process_comms(true)
2. Set QoS queue size to 0 with "keep all" behavior (maybe even SystemDefaultsQoS??)
3. Create a publisher

Expected behavior

At the very least an actionable error message, but preferably a functioning node and publisher

Actual behavior

terminate called after throwing an instance of 'std::invalid_argument'
  what():  size must be a positive, non-zero value
...
#6  0xXXXXXXXXXXXXXXXX in rclcpp::mapped_ring_buffer::MappedRingBuffer<geometry_msgs::msg::Twist_<std::allocator<void> >, std::allocator<geometry_msgs::msg::Twist_<std::allocator<void> > > >::MappedRingBuffer (this=0xXXXXXXXXXXXX, size=0, 
    allocator=std::shared_ptr<std::allocator<geometry_msgs::msg::Twist_<std::allocator<void> > >> (use count 2, weak count 0) = {...}) at /opt/ros/master/install/include/rclcpp/mapped_ring_buffer.hpp:82
...

Additional information

Workaround is to set a non-zero queue size and use "keep last" behavior.

[cottsay]

@rotu - feel free to add anything I might have missed.

[ivanpauno]

The problem here, is that the size of the MappedRingBuffer used for intraprocess communication, it's created with the history depth value. When using KeepAll, we should rely in another value for sizing the MappedRingBuffer.
I think internally DDS use a value based on RESOURCE_LIMITS for sizing its buffers with KeepAll qos policy, so we should do something similar. Actually, that information isn't accessible in rclcpp and I'm not completely sure what is the "correct" value to use.
Maybe, just throwing a nice error saying "KEEP_ALL durability policy not supported with intra process communication" is enough for the moment.
@wjwwood any thoughts?

[wjwwood]

I asked @cottsay to open it, so I'm familiar with the cause of the issue.

---

maybe even SystemDefaultsQoS??

SystemDefaultsQoS shouldn't be keep all unless you configure it as such with an external XML file using the configurations provided by the middleware (not via ROS), but perhaps the default might be that. Either way, I don't recommend anyone use that unless you explicitly want to use the external overrides.

I think internally DDS use a value based on RESOURCE_LIMITS for sizing its buffers with KeepAll qos policy, so we should do something similar. Actually, that information isn't accessible in rclcpp and I'm not completely sure what is the "correct" value to use.

We need to add the resource limits to our QoS, otherwise KEEP_ALL cannot be configured at all. Now that we support IDL and therefore keyed messages (and therefore we could have instances), we need the resource limits as well as that's how you can configure the number of allowed instances on a subscription (DataReader).

I don't know if there's a ticket for that or not.

Maybe, just throwing a nice error saying "KEEP_ALL durability policy not supported with intra process communication" is enough for the moment.

That plus an error if it is KEEP_LAST depth 0. I think better errors is the right way to go for now.

[ivanpauno]

That plus an error if it is KEEP_LAST depth 0. I think better errors is the right way to go for now.

Ok, mind if I open a PR with that?

[wjwwood]

Sure.