From c28bab62d1199affd3a0fe349da8e6d9c30b8432 Mon Sep 17 00:00:00 2001 From: Devin Bonnie <47613035+dabonnie@users.noreply.github.com> Date: Wed, 5 Aug 2020 11:51:43 -0700 Subject: [PATCH] Fix exception thrown given invalid arguments with compression enabled (#488) (#489) * Remove exception throw in dtor Update logging errors to avoid duplication Signed-off-by: Devin Bonnie * Cleanup reset logic Add reset documentation Signed-off-by: Devin Bonnie * Fix cpplint error Signed-off-by: Devin Bonnie * Fix docstring Signed-off-by: Devin Bonnie --- .../sequential_compression_writer.hpp | 4 ++++ .../sequential_compression_writer.cpp | 12 ++++-------- .../test_sequential_compression_writer.cpp | 1 + 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/rosbag2_compression/include/rosbag2_compression/sequential_compression_writer.hpp b/rosbag2_compression/include/rosbag2_compression/sequential_compression_writer.hpp index ecdca9d5df..fb1a930cdf 100644 --- a/rosbag2_compression/include/rosbag2_compression/sequential_compression_writer.hpp +++ b/rosbag2_compression/include/rosbag2_compression/sequential_compression_writer.hpp @@ -75,6 +75,10 @@ class ROSBAG2_COMPRESSION_PUBLIC SequentialCompressionWriter const rosbag2_cpp::StorageOptions & storage_options, const rosbag2_cpp::ConverterOptions & converter_options) override; + /** + * Attempt to compress the last open file and reset the storage and storage factory. + * This method must be exception safe because it is called by the destructor. + */ void reset() override; /** diff --git a/rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp b/rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp index bd756a2156..3008fd3271 100644 --- a/rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp +++ b/rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp @@ -118,8 +118,8 @@ void SequentialCompressionWriter::open( throw std::runtime_error{"No storage could be initialized. Abort"}; } - if (storage_options.max_bagfile_size != 0 && - storage_options.max_bagfile_size < storage_->get_minimum_split_file_size()) + if (max_bagfile_size_ != 0 && + max_bagfile_size_ < storage_->get_minimum_split_file_size()) { std::stringstream error; error << "Invalid bag splitting size given. Please provide a value greater than " << @@ -134,11 +134,7 @@ void SequentialCompressionWriter::open( void SequentialCompressionWriter::reset() { - if (!base_folder_.empty()) { - if (!compressor_) { - throw std::runtime_error{"Compressor was not opened!"}; - } - + if (!base_folder_.empty() && compressor_) { // Reset may be called before initializing the compressor (ex. bad options). // We compress the last file only if it hasn't been compressed earlier (ex. in split_bagfile()). if (compression_options_.compression_mode == rosbag2_compression::CompressionMode::FILE && @@ -208,7 +204,7 @@ void SequentialCompressionWriter::remove_topic( void SequentialCompressionWriter::compress_last_file() { if (!compressor_) { - throw std::runtime_error{"Compressor was not opened!"}; + throw std::runtime_error{"compress_last_file: Compressor was not opened!"}; } const auto to_compress = rcpputils::fs::path{metadata_.relative_file_paths.back()}; diff --git a/rosbag2_compression/test/rosbag2_compression/test_sequential_compression_writer.cpp b/rosbag2_compression/test/rosbag2_compression/test_sequential_compression_writer.cpp index b1b9c7c763..c1e06088e2 100644 --- a/rosbag2_compression/test/rosbag2_compression/test_sequential_compression_writer.cpp +++ b/rosbag2_compression/test/rosbag2_compression/test_sequential_compression_writer.cpp @@ -89,6 +89,7 @@ TEST_F(SequentialCompressionWriterTest, open_throws_on_invalid_splitting_size) ON_CALL(*storage_, get_minimum_split_file_size()).WillByDefault(Return(min_split_file_size)); auto storage_options = rosbag2_cpp::StorageOptions{}; storage_options.max_bagfile_size = max_bagfile_size; + storage_options.uri = "foo.bar"; auto sequential_writer = std::make_unique( compression_options,