Fixes bag not closing properly (#1586)

[yschulz]

This PR fixes #1586.
Basically, destruction of the storage_factory_ does not allow the writer to open another bag file after it got closed. I also don't think that it is necessary to destroy the factory in the first place as it does not get any data from the writer and its unique anyway. What did prevent the writer to work as expected was keeping track of existing topics in the map topics_names_to_info_ but that can be easily cleared on close.

I don't know if the test is in the maintainers way of doing things, please let me know.

[yschulz]

Converted this pr to a draft because it breaks record_end_to_end_with_splitting_metadata_contains_all_topics test. Not exactly sure why, but clearing the topics map results in the recorder not writing the topic info to the meta data, despite being called after the dump.
I suspect the cache might be the reason here, creating some sort of racecondition closing a split bag. But I don't seem to see where the issue comes from directly. Maybe somebody more familiar with the repo knows better.

[yschulz]

Well, forget my last comment, haha. I got misguided by [this comment].(https://github.com/ros2/rosbag2/blob/humble/rosbag2_cpp/include/rosbag2_cpp/writers/sequential_writer.hpp#L143)

Anyhow, the problem was that the recorder (the only place that explicitly calls the close method in the project) overwrote the meta data file with the cleared info when the object ran out of scope. It now checks if the storage object is still alive before calling it again in the destructor.

[MichaelOrlov]

@yschulz Could you please write a clean and concise RCA (Root Cause Analysis)?

[yschulz]

Hi all,
sorry for the chaotic PR, I was figuring out the repo as I went. I am also not sure I get exactly what you want from me, but let me try to summarize and be more consise:

Bug (#1586):

Problem

When using the rosbag2_cpp api and creating an instance of a writer, the executable segfaults when the writer is consecutively closed and opened.

Expected behavior:
When I open the writer with a valid folderpath, write messages to it and close it, I expect to be able to open the writer again with a different folder path and write messages to it. I expect this operation would create two individual bags at different locations corresponding to the folderpath specified when opening the writer.

Actual behavior:
When the writer is opened a second time, the program segfaults. A folder is created in the process, but the storage does not log that any bag was opened.

SSCCE:
Either checkout to this commit where I extended the test to write two bags or use this example:

#include <rclcpp/rclcpp.hpp>
#include <rosbag2_cpp/writer.hpp>
#include <string>

int main(int argc, char* argv[]) {
    rclcpp::init(argc, argv);

    std::string topic = "/test_topic";
    std::string type = "std_msgs/msg/bool";

    std::shared_ptr<rclcpp::SerializedMessage> msg = std::make_shared<rclcpp::SerializedMessage>(2);
    std::unique_ptr<rosbag2_cpp::Writer> writer = std::make_unique<rosbag2_cpp::Writer>();

    std::string out_1 = ""/*desired_path_first_bag*/;
    std::string out_2 =  ""/*desired_path_second_bag*/;

    auto time_stamp = rclcpp::Time(1); 

    writer->open(out_1);
    writer->write(msg, topic, type, time_stamp);
    writer->close();

    writer->open(out_2);
    writer->write(msg, topic, type, time_stamp);
    writer->close();

    rclcpp::shutdown();
    return 0;
}

Fix

The problem was two-fold and revealed another bug later:

1. The segfault
   The segfault was a result of destruction of the storage_factory in the close method. The factory is passed by the interface to what I assume is allowing different strategies to handle the bags. This creates a member instance of the storage factory inside the writer class. Therefore, this object needs to survive the closing of the bag, so it can open another bag down the line. Removing this line fixed the segfault.

2. Tracking topics after closing the writer
   Even if the survival of the factory is ensured, the writing of the second bag will throw from the storage instance, complaining that topics first need to be created. Since the writer class finds the topic, that it tries to write, already within the map, there is no call to the storage create_topic method. Therefore it has to be cleared before a new bag is opened.

3. New bug: No topics are present in the metadata.yaml when closing the writer by the method close
   Most often, the closing of the writer is triggered by just letting it run out of scope, so the destructor triggers close(). However, if the writer was closed by calling close() directly, the metadata file was already written and the topic info map was cleared. Letting the writer now go out of scope will trigger close() once more and the the metadata file be overwritten with the now empty topic info map. Regardless of my changes, the metadata file would be woverwritten every time someone would call close() and let the writer run out of scope, which is generally not desired behavior I assume.

I hope this makes things more clear, if you need anything else, please let me know.

[fujitatomoya]

overall i think this is right thing to do. thank you for the explanation and effort.

i think that we should apply the same fixes to SequentialCompressionWriter including tests?

[fujitatomoya]

i think this is right thing to do. storage_factory_ lifetime should be aligned with Writer until it is destructed.

[fujitatomoya]

how about topic_names_to_message_definitions_ i think this should be cleared at the same time here?
IMO, we need to clear the all member variables that are not reset in the open method for the fresh start.

[MichaelOrlov]

@yschulz Thanks for trying to fix this issue and explanation about how to reproduce it.
The first thing first. We need to make a fix on the Rolling first then backport it if possible to the Humble.
I agree with @fujitatomoya that we need to think more thoroughly about what else can go wrong if closing and opening the writer again without destruction and address the possible issues as well.
In rosbag2 itself we don't have this scenario when opening the writer more than once without destruction.

[MichaelOrlov]

Don't bother creating a new serialized message, I would recommend using the same serialized_msg2 already created upper.

[MichaelOrlov]

@yschulz It seems that early mentioned issue rosbag2_cpp::Writer can not close correctly, rasing "Segmentation fault(core dumped)" Error #1586 has different scenario for reproduction.

In this PR you described that segfault happened when open-close writer twice or at least open it one more time. However, in the
rosbag2_cpp::Writer can not close correctly, raising "Segmentation fault(core dumped)" Error #1586 mentioned that segfault happened when closing or destroying writer on the first iteration.
If this is true, I would encourage you to create a separate issue related to this PR and copy the problem description from your previous post #1590 (comment)