Transforms published with the same timestamp accumulate indefinitely in all listeners

[beiju]

tf2's TimeCache deletes cached transforms when they are more than X seconds older than the newest transform. This means that if any node in the ROS system regularly publishes a transform with an unchanging timestamp (e.g. ros::Time(0)), all listeners in all nodes will increase in memory usage without bound. In practice this can cause tens of gigabytes per day.

I suggest changing the buffer so that inserting a transform with timestamp equal to an existing transform's timestamp replaces the existing transform instead of adding an additional entry. The implementation already has to search for the insertion location, so this is very minimal extra work. In the current implementation the existing transform will never be used again, so no information is lost.

If for some reason you don't want to do that, I suggest at least adding a warning the first time a transform with duplicate time stamp is received. This is a difficult issue to debug, especially considering the symptom appears in a different node than the cause.

Either way I'm happy to submit a PR if you approve.

[tfoote]

Hmm, in general you're not supposed to rebroadcast data with the same timestamp. I would consider that a bug in the other code. If you want to do something like that you should be using a static transform. Note that the special capabilities of time zero are only available on query not for the publishing. Time zero represents time not initialized and nothing should actively have that timestamp.

However we shouldn't allow the buffer to grow unbounded if someone does publish the same timestamp. The design of the tf datastructure is that it's a record of the streamed state over time and that sequential queries should be deterministic except for refinements due to delayed data. I would suggest that instead of updating the value and erroring. We drop the new value and error.

If we allow updating I could see someone starting to do a mass republishing of content and trying to rewrite all values in the buffer since you can force a collision with any elements in the array. This is really dangerous though as if you're rewriting history and you miss a few samples you then will start interpolating over the two different time series which would be problematic.

To have a "rewritable history" I think that we'd want to support a different datastructure similarly to how we publish tf_static in a separate datastructure a full spline series or something similar could be published for each frame and allow updating queries in the past in an holistic manner. But I am digressing from the topic at hand except to confirm that I'd like to keep the unrevisable status of the buffer.

If you could submit a PR that checks on insert:

• If timestamp == 0 -> drop sample and provide throttled error
• If timestamp already in buffer -> drop sample and provide throttled error

[pbeeson]

I independently ran into this Issue this week and submitted PR #425 (indigo/kinetic) and #426 (melodic) to fix it.

[pbeeson]

I wish I would have seen this issue. It might have saved me a couple of hours of tracing things into cache.cpp. But at least ist sounds like my fix is exactly what you proposed, so that's good.

[beiju]

Thanks for the patch pbeeson! That's about what I had planned to do. Sorry it took so long to get back to this.