fix sequential reader rollover-to-next-file

[lihui815]

• has_next() - calls recursively until next message OR no next file found
• read_next() - calls has_next() to ensure necessary rollovers are made if necessary

Signed-off-by: Sonia Jin diegothemuich@gmail.com

[emersonknapp]

Nice fix

[emersonknapp]

• Linux
• Linux-aarch64
• macOS
• Windows

[MichaelOrlov]

@emersonknapp @lihui815 I am sorry for my late response to this closed issue, but IMO it was not a real issue it was working as designed. We just didn't have Doxygen documentation for these API.
I believe it was designed this way on purpose for the sake of performance to avoid calling has_next() twice.
We are calling has_next() and read_next() always sequentially in
Player::enqueue_up_to_boundary(uint64_t boundary)

void Player::enqueue_up_to_boundary(uint64_t boundary)
{
  rosbag2_storage::SerializedBagMessageSharedPtr message;
  for (size_t i = message_queue_.size_approx(); i < boundary; i++) {
    if (!reader_->has_next()) {
      break;
    }
    message = reader_->read_next();
    message_queue_.enqueue(message);
  }
}

and this is only one place where we are calling it.

I would suggest to revert this PR and add relevant Doxygen comment to the read_next() API that if it would be called without checking that has_next() returning true it will be undefined behavior.

[MichaelOrlov]

After some detailed consideration I will step back. Sorry for disturbing.
We actually will need this extra call for has_next() inside read_next() to make sure that seek timestamp will be applied correctly and calling one more time for the reader->has_next() will translates to the one more call to the storage->has_next() call which is going to check that iterator doesn't point to the end and it will not affect performance.