Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes bag not closing properly (#1586) #1590

Closed
wants to merge 4 commits into from
+45 −4
Closed

Fixes bag not closing properly (#1586) #1590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dd4db46
adds brief test of opening a new bag after closing the writer before
yschulz Mar 17, 2024
2f570e7
fixes bug (#1586). Avoids destruction of the factory and instead clea…
yschulz Mar 17, 2024
e495ec2
checks if writer has already been closed before closing when running …
yschulz Mar 17, 2024
f279046
adds uncrustify style
yschulz Mar 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,10 @@ SequentialWriter::SequentialWriter(

SequentialWriter::~SequentialWriter()
{
close();
// only close writer if bag is open to prevent overwrite
if (storage_) {
close();
}
}

void SequentialWriter::init_metadata()
Expand Down Expand Up @@ -161,8 +164,9 @@ void SequentialWriter::close()
metadata_io_->write_metadata(base_folder_, metadata_);
}

storage_.reset(); // Necessary to ensure that the storage is destroyed before the factory
storage_factory_.reset();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// destroy current storage object and clear active track map for topic metadata
storage_.reset();
topics_names_to_info_.clear();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

void SequentialWriter::create_topic(const rosbag2_storage::TopicMetadata & topic_with_type)
Expand Down
39 changes: 38 additions & 1 deletion rosbag2_tests/test/rosbag2_tests/test_rosbag2_cpp_api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,10 @@ TEST(TestRosbag2CPPAPI, minimal_writer_example)
serialization.serialize_message(&test_msg, &serialized_msg);

auto rosbag_directory = rcpputils::fs::path("test_rosbag2_writer_api_bag");
auto rosbag_directory_next = rcpputils::fs::path("test_rosbag2_writer_api_bag_next");
// in case the bag was previously not cleaned up
rcpputils::fs::remove_all(rosbag_directory);
rcpputils::fs::remove_all(rosbag_directory_next);

{
rosbag2_cpp::Writer writer;
Expand Down Expand Up @@ -87,7 +89,23 @@ TEST(TestRosbag2CPPAPI, minimal_writer_example)
// writing a non-serialized message
writer.write(test_msg, "/a/ros2/message", rclcpp::Clock().now());

// close on scope exit
// close as prompted
writer.close();

// open a new bag with the same writer
writer.open(rosbag_directory_next.string());

// another simple serialized message
std::shared_ptr<rclcpp::SerializedMessage> serialized_msg3 =
std::make_shared<rclcpp::SerializedMessage>();
serialization.serialize_message(&test_msg, serialized_msg3.get());
Comment on lines +98 to +101
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// write same topic to different bag
writer.write(
serialized_msg3, "/yet/another/topic", "test_msgs/msg/BasicTypes",
rclcpp::Clock().now());

// close by scope
}

{
Expand All @@ -114,6 +132,25 @@ TEST(TestRosbag2CPPAPI, minimal_writer_example)
// close on scope exit
}

// read the other written bag
{
rosbag2_cpp::Reader reader;
std::string topic;
reader.open(rosbag_directory_next.string());
ASSERT_TRUE(reader.has_next());

auto bag_message = reader.read_next();
topic = bag_message->topic_name;

TestMsgT extracted_test_msg;
rclcpp::SerializedMessage extracted_serialized_msg(*bag_message->serialized_data);
serialization.deserialize_message(
&extracted_serialized_msg, &extracted_test_msg);

EXPECT_EQ(test_msg, extracted_test_msg);
EXPECT_EQ("/yet/another/topic", topic);
}

// alternative reader
{
rosbag2_cpp::Reader reader;
Expand Down