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 <dbbonnie@amazon.com> * Cleanup reset logic Add reset documentation Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Fix cpplint error Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Fix docstring Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
ros2 · Aug 5, 2020 · c28bab6 · c28bab6
1 parent e380101
commit c28bab6
Show file tree

Hide file tree

Showing 3 changed files with 9 additions and 8 deletions.
diff --git 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
@@ -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
@@ -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<rosbag2_compression::SequentialCompressionWriter>(
     compression_options,