Allow mixing latched and unlatched publishers.

[mikepurvis]

This is a naive change (so far) that just removes the check against mixing latched and unlatched publishers and updates the test added in #1544 so that it covers this scenario (it still passes). As far as I can tell, this should be totally safe and in line with the design intent of the original change:

1. Each Publisher instance now stores its own latched/unlatched status, and its own last_message_ pointer, rather than this being at the TopicManager/singleton level.
2. When a new subscriber connects, the Publication::peerConnect method iterates its list of callbacks, which will call the push_latched_message_ std::function for any that have it defined.

Would love to hear from @meyerj if there's something lurking here that I've missed.

Fixes #1966.

[meyerj]

Would love to hear from @meyerj if there's something lurking here that I've missed.

Just commented in #1966 (comment):

Allowing mixed latched and volatile (non-latching) publishers in a single node is error-prone. The singleton publication is either reporting itself as latching or not in the TCPROS connection header. Normal subscribers do not consider this flag and mixed publishers indeed behave as intended.

But rosbag record does not see individual publishers. The bag only contains the information that topic /tf_static advertised by /node_a was latched or not. If the same topic would have been advertised by /node_b as volatile, that is still okay and can be recorded correctly. But during rosbag play there is again only one singleton publication for both, which is latched or not depending on which connection gets advertised first (Re-recording would not work anymore and the connections would be merged into one. That is probably a rare case and acceptable.).

Overall, I agree that removing the check is a viable solution for rosbag play to at least work for the case of two different nodes (i.e. connections) advertising heterogeneously on the same topic. On the other hand, if there would indeed be multiple latching publishers or even a mix of latching and non-latching publishers within the same node, rosbag record cannot record this situation anyway and only pushes the last message from any of the original publishers to new subscribers. This is still an unresolved problem, especially when recording and replaying /tf_static.

[mikepurvis]

@meyerj Thanks for the in-depth explanation. Given these situations:

1. Topic with one latched publisher each in multiple processes.
2. Topic with multiple latched publishers in the same process (or multiple processes).

My understanding is that prior to #1544, case 1 would would work initially but not play back from bag properly, and case 2 would not even work initially (you'd get whichever was the most recent message, rather than one per latched publisher).

With #1544, case 1 is now able to be played back from bag—even historical bags, curiously, since the limitation was always just in rosbag's ability to create multiple latched publishers at playback time. Case 2 now works initially, but still can't be accurately bagged since the TCPROS protocol only sees processes, not individual publishers within them.

And since rosbag playback of case 1 is itself case 2, rebagging that will also mess it up (I believe rosbag filter dumps the connection info too, though possibly for different reasons?).

Anyway, I think all that makes sense, but this is the bit that I'm hazy on:

Allowing mixed latched and volatile (non-latching) publishers in a single node is error-prone. The singleton publication is either reporting itself as latching or not in the TCPROS connection header. Normal subscribers do not consider this flag and mixed publishers indeed behave as intended.

As you say, the only/main case where the flag matters is for special subscribers like rosbag record and topic_tools/relay. Since they didn't do the right thing previously, and still can't do it now unless there are protocol changes, I think the only realistic thing to do is just report latched at the process level if any of the underlying publishers are latched— I think that's effectively what the old behaviour was anyway, that as soon as one latched Publisher was created, the underlying singleton marked the topic as such and that was that.

This obviously leaves a race where a subscriber may see the topic as unlatched because the latched publisher in that same process hasn't been instantiated yet. But if I'm correct about the old behaviour then that at least wouldn't be a regression.

[meyerj]

@mikepurvis Yes, that's a concise summary of the situation, with the following correction:

I think that's effectively what the old behaviour was anyway, that as soon as one latched Publisher was created, the underlying singleton marked the topic as such and that was that.

The first publisher created in a roscpp process on a given topic decides whether the publication is advertised as latching or not in the connection header:

From topic_manager.cpp:

 bool TopicManager::advertise(const AdvertiseOptions& ops, const SubscriberCallbacksPtr& callbacks)
 {
   // [...]
   pub = lookupPublicationWithoutLock(ops.topic);
   // [...]

   if (pub)
   {
     // [...]

-    if (pub->isLatched() != ops.latch)
-    {
-      ROS_ERROR("Tried to advertise on topic [%s] with latch=%d but the topic is already advertised with latch=%d",
-                ops.topic.c_str(), ops.latch, pub->isLatched());
-      return false;
-    }

     pub->addCallbacks(callbacks);

     return true;
   }

   pub = PublicationPtr(boost::make_shared<Publication>(ops.topic, ops.datatype, ops.md5sum, ops.message_definition, ops.queue_size, ops.latch, ops.has_header));
   pub->addCallbacks(callbacks);
   advertised_topics_.push_back(pub);
}

So if there is already a publisher on the given topic and the check is removed as suggested in this PR, then ops.latch is ignored for the second and subsequent publishers. The latching Publisher instances themselves will still push their latest sample to new subscribers in their push_latched_message_ callback, with or without reporting that in the connection header. Or in other words the header flag and as a result the behavior of "special subscribers" is undefined if the check was removed.

As you also suggested, maybe it would be best to remove or deprecate the Publication::latch_ member and return true from Publication::isLatched() and redundant Publication::isLatching() if and only if there is at least one latching Publisher (from V_Callback callbacks)? On the other hand existing subscribers won't be notified when a new latching publisher is added to an existing publication, or when the last latching publisher has been removed. And neither of rosbag record and topic_tools/relay works without hidden pitfalls if they are multiple latched publishers within the same node.

The standard example for a broken situation is a node/process that has multiple StaticTransformPublisher instances and hence multiple /tf_static publishers. For nodes subscribing online, that use case is fixed now with #1544. But rosbag record /tf_static and later rosbag play will only relatch the transforms message that was recorded last.

Anyway, I think all that makes sense, but this is the bit that I'm hazy on:

Allowing mixed latched and volatile (non-latching) publishers in a single node is error-prone. The singleton publication is either reporting itself as latching or not in the TCPROS connection header. Normal subscribers do not consider this flag and mixed publishers indeed behave as intended.

I agree. The issue cannot be fixed in a nice and sane way anyway without changing the TCPROS protocol. So if removing the check at least resolves the use case of rosbag play with mixed latching and non-latching publishers, then it is probably fine to do so. So 👍.

On the other hand, I still cannot think of a good use case where mixing latching and non-latching publishers on the same topic, even in different nodes, is actually required. If that would be strictly forbidden - and maybe even enforced by the ROS master - then there would be no problem with an inconsistently reported latching state in the connection header, and rosbag play would also not trigger the regression reported in #1966, because you cannot record a bag file with conflicting connection infos anymore. However, even with such a strict policy rosbag play does not correctly latch multiple messages originating from multiple publishers within the same node. Could you elaborate on your use case and how you ended up with that bag file?

[meyerj]

It should also be noted that all above is only valid for roscpp. Other client libraries will behave differently.

For rospy @mgrrx proposed a patch in #146 (comment) (magazino@7c6f5b0), which originally motivated #1544. I still think that this commit should be applied to rospy to achieve at least a consistent behavior of rospy and roscpp nodes having multiple latched publishers (e.g. multiple tf2_ros.StaticTransformBroadcaster).

[mikepurvis]

@meyerj Have implemented now the behaviour where the singleton is marked as latched if any of the instances are. Ideally this would have been just done in the isLatched accessor, but since that accessor was implemented in the header, it would have been inlined and thus broken ABI. So instead I leave the accessor as-is and update the existing member.

Let me know if this approach is acceptable to you.

---

Regarding the question of valid use-cases, I'm not sure that there are any either, and I've cleaned up the ones I'm aware of in our codebase, but we still have historical bags where a rogue node was publishing rosout as latched or whatever. We also have a situation where bags that had been processed for use with webviz included collapsing together the tf (unlatched) and tf_static (latched) topics, which is now a source of error.

At the end of the day though, I just don't like the idea of an arbitrary runtime incompatibility on the level of MD5 breakage that's buried in code and has to be navigated like this, especially when it's something that bites later on in development (when playing a bag, when combining nodelets, etc).

[meyerj]

@meyerj Have implemented now the behaviour where the singleton is marked as latched if any of the instances are. Ideally this would have been just done in the isLatched accessor, but since that accessor was implemented in the header, it would have been inlined and thus broken ABI. So instead I leave the accessor as-is and update the existing member.

Let me know if this approach is acceptable to you.

Yes, thanks for the update. It is a reasonable choice to keep the existing latch_ flag up-to-date in addCallbacks() and removeCallbacks() instead of removing it. This likely happens less frequently than new incoming connections from subscribers, where isLatching() is checked for the connection header in transport_subscriber_link.cpp:114.

Strictly speaking latch_ must be protected by the callbacks_mutex_ now, because the flag is not immutable anymore after the construction of a Publication instance. Or it needs to be of type std::atomic<bool>, but that would break the ABI, too. I guess for now it is acceptable that isLatched() and isLatching() eventually report an outdated state due to cache coherency issues, because they do not reflect the full truth anyway if there are mixed latching and non-latching subscribers. But it would be possible to move the implementation to the source and to add a scoped lock of the callbacks_mutex_ without breaking the ABI. There is just no guarantee that code compiled against a previous version of the header did not inline the method and uses the implementation without the lock.

[mikepurvis]

@meyerj Thanks for pointing that out; I've made the suggested change.

@dirk-thomas I think this is good to go in, when you're able to look.

[dirk-thomas]

Thanks for the patch!