Check for nullptrs when pushing new messages to the message cache

[jncfa]

Description

Adds a simple check to the message cache circular buffer when pushing nullptr messages.
Changed CircularMessageCache to report when dropping messages (null or too large for buffer).

Fixes #2115

Is this user-facing behavior change?

As it only adds a check if the message is a nullptr, this should not change any normal runtime behavior as this should always trigger a segmentation fault or at minimum UB.

Did you use Generative AI?

No

Additional Information

[MichaelOrlov]

Sorry, but Nop. Not this way at least.
Please refer to my original response in the relevant issue #2115 (comment).

In essence, if we return false from the push method, it means that the message was lost, and this message drop will be reported on the upper layer and will be counted in statistics. This is incorrect because CircularMessageCache drops messages by design when the message queue is full, and we should not report messages lost on the upper layer.

[jncfa]

Sorry, but Nop. Not this way at least. Please refer to my original response in the relevant issue #2115 (comment).

In essence, if we return false from the push method, it means that the message was lost, and this message drop will be reported on the upper layer and will be counted in statistics. This is incorrect because CircularMessageCache drops messages by design when the message queue is full, and we should not report messages lost on the upper layer.

is throwing more reasonable or would you rather just not have this in at all? asking since you already closed the original issue for it

if you don't see any added value by adding an exception with a custom message I'll just close the PR

[MichaelOrlov]

Hi @jncfa, I've just spent some time making a deeper analysis of the CircularCache and its underlying CircularBuffer.
It doesn't make sense to check for nullptr inside the CircularMessageCache::push() since we are not dereferencing it there.
However, it does make sense to check for nullprt inside bool MessageCacheCircularBuffer::push(CacheBufferInterface::buffer_element_t msg) where we are dereferencing those shared pointer.
Since we already have code that drops messages in the MessageCacheCircularBuffer::push(..) due to the message size exceeding the snapshot buffer size. Could you please do the following:

1. Remove check for nullptr in the CircularMessageCache::push(..)
2. Change CircularMessageCache::push(..) to return a result of the underlying push method from the producer.

     bool CircularMessageCache::push(std::shared_ptr<const rosbag2_storage::SerializedBagMessage> msg)
     {
       std::lock_guard<std::mutex> cache_lock(producer_buffer_mutex_);
       return producer_buffer_->push(msg);
     }
   

3. Add check for nullptrs in the MessageCacheCircularBuffer::push

   bool MessageCacheCircularBuffer::push(CacheBufferInterface::buffer_element_t msg)
    {
      if (!msg || !msg->serialized_data) {
        ROSBAG2_CPP_LOG_ERROR("Attempted to push null message into circular buffer. Dropping message!");
        return false;
      }
      // Drop message if it exceeds the buffer size
      if (msg->serialized_data->buffer_length > max_bytes_size_) {
        ROSBAG2_CPP_LOG_WARN("Last message exceeds snapshot buffer size. Dropping message!");
        return false;
      }
    ...
   

[MichaelOrlov]

@jncfa Thanks for the follow-up.
Now looks good with one minor adjustment. Can you please update Doxygen comments for the push methods in the header files?

[MichaelOrlov]

@ros-pull-request-builder retest this please

[fujitatomoya]

lgtm