Bugfix for writer not being able to open again after closing #1599

yschulz · 2024-03-30T09:41:21Z

Fixes Writer cannot be opened again after closing #1598
After discussion in Fixes bag not closing properly (#1586) #1590 I verified the issue in and reapplied the changes and comments to rolling. To reiterate, the storage_factory needs to persist beyond the call to close a current bag. On the other hand, all containers that were built during the recording have to be cleared before opening another bag.

yschulz · 2024-03-30T09:45:30Z

I keep it as a draft until @MichaelOrlov and @fujitatomoya can verify what all can be attached to a writers lifetime and what needs to be reset. I am especially not sure about the compressed writer, since I haven't spend enough time looking at that one. Thank you guys for your comments and review, I guess we can close the other PR?

fujitatomoya · 2024-04-01T03:35:02Z

@yschulz so this replaces #1590, right? in that case, you can close #1590, and let us know this one is ready to review.

yschulz · 2024-04-01T19:35:34Z

@fujitatomoya done, and yes please :)

I had a closer look at the compression writer and as to my impression, there is no need to clear anything regarding the compression functionality. However, I saw that the closing of all the storage functionality can be directly delegated to the base class (as it is for opening and writing anyway). Also, spotted an unused member, maybe a relict from before the threaded approach, you may know more.

rosbag2_cpp/include/rosbag2_cpp/writers/sequential_writer.hpp

rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp

yschulz · 2024-05-06T05:41:19Z

Thanks for the review and addressing the changes directly.

MichaelOrlov · 2024-05-06T06:15:58Z

@yschulz Sorry for taking a long time to review. It was a rush time before Jazzy code freeze. And changes per this PR were non-trivial for review. Was need to verify that we will not break anything.

MichaelOrlov · 2024-05-06T06:18:38Z

Gist: https://gist.githubusercontent.com/MichaelOrlov/2677c3394586488cd9a777215205eff9/raw/4e54566b952d8218af61493f02a1e01394e567bf/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_cpp rosbag2_compression rosbag2_tests
TEST args: --packages-above rosbag2_cpp rosbag2_compression rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13815

Linux
Linux-aarch64
Windows

fujitatomoya · 2024-05-06T16:07:23Z

@MichaelOrlov thanks for checking this, i will take a look at this today.

MichaelOrlov · 2024-05-06T16:24:53Z

Re-run Windows CI
It seems the previous failure is unrelated to the changes from this PR.

C:\ci\ws\src\yschulz\rosbag2\rosbag2_storage\include\rosbag2_storage/yaml.hpp(28,10): fatal error C1083: Cannot open include file: 'yaml-cpp/yaml.h': No such file or directory (compiling source file C:\ci\ws\src\yschulz\rosbag2\rosbag2_storage\src\rosbag2_storage\metadata_io.cpp) [C:\ci\ws\build\rosbag2_storage\rosbag2_storage.vcxproj]

Windows

MichaelOrlov · 2024-05-06T18:21:22Z

@clalancette Out of curiosity if this is a known issue

C:\ci\ws\src\yschulz\rosbag2\rosbag2_storage\include\rosbag2_storage/yaml.hpp(28,10): fatal error C1083: Cannot open include file: 'yaml-cpp/yaml.h': No such file or directory (compiling source file C:\ci\ws\src\yschulz\rosbag2\rosbag2_storage\src\rosbag2_storage\metadata_io.cpp) [C:\ci\ws\build\rosbag2_storage\rosbag2_storage.vcxproj]

Or we missing something like find_package(yaml-cpp REQUIRED) in CMake file?

clalancette · 2024-05-06T18:34:39Z

@clalancette Out of curiosity if this is a known issue

No, not a known issue. This succeeded on the nightlies: https://ci.ros2.org/view/nightly/job/nightly_win_rel/3037/ .

My best guess is that this PR needs to be rebased onto the latest rosbag2 code to get #1605

fujitatomoya

lgtm with green CI

…definition in sequential writer test for multiple open operations. Also clears topic_names_to_message_definitions_ and handles message_definitions_s underlying container similarly. Lastly, also avoids reset of factory in the compression writer, adds unit test there too. Signed-off-by: Yannick Schulz <yschulz854@gmail.com>

…delegates rest of the closing behavior to the base class in close method, as it is handled in the open and write methods of the compression writer Signed-off-by: Yannick Schulz <yschulz854@gmail.com>

- message_definitions_ was intentionally allocated on the stack and should persist between writer close() and open() because it represents cache for message definitions which is not changes. Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

- Rationale: To avoid race conditions if open(..) and close() could be ever be called from different threads. Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

MichaelOrlov · 2024-05-06T19:24:30Z

Re-run Windows CI after rebasing on the latest Roling

Windows

MichaelOrlov · 2024-05-06T21:59:16Z

Ok. CI is green after rebase. Merging then.

MichaelOrlov · 2024-05-06T22:00:34Z

https://github.com/Mergifyio backport jazzy

mergify · 2024-05-06T22:00:39Z

backport jazzy

✅ Backports have been created

#1639 Bugfix for writer not being able to open again after closing (backport #1599) has been created for branch jazzy

* re-applies fixes from #1590 to rolling. Also removes new message definition in sequential writer test for multiple open operations. Also clears topic_names_to_message_definitions_ and handles message_definitions_s underlying container similarly. Lastly, also avoids reset of factory in the compression writer, adds unit test there too. Signed-off-by: Yannick Schulz <yschulz854@gmail.com> * removes unused compressor_ member from compresser writer class. Also delegates rest of the closing behavior to the base class in close method, as it is handled in the open and write methods of the compression writer Signed-off-by: Yannick Schulz <yschulz854@gmail.com> * Remove unrelated delta - message_definitions_ was intentionally allocated on the stack and should persist between writer close() and open() because it represents cache for message definitions which is not changes. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Don't call virtual methods from destructors Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Cleanup 'rosbag2_directory_next' after the test run Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Protect Writer::open(..) and Writer::close() with mutex on upper level - Rationale: To avoid race conditions if open(..) and close() could be ever be called from different threads. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Bugfix for WRITE_SPLIT callback not called for the last compressed file Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Bugfix for lost messages from cache when closing compression writer Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Use dedicated is_open_ flag instead of relying on storage_ Signed-off-by: Michael Orlov <michael.orlov@apex.ai> --------- Signed-off-by: Yannick Schulz <yschulz854@gmail.com> Signed-off-by: Michael Orlov <michael.orlov@apex.ai> Co-authored-by: Michael Orlov <michael.orlov@apex.ai> (cherry picked from commit 20cfc02)

…1639) * re-applies fixes from #1590 to rolling. Also removes new message definition in sequential writer test for multiple open operations. Also clears topic_names_to_message_definitions_ and handles message_definitions_s underlying container similarly. Lastly, also avoids reset of factory in the compression writer, adds unit test there too. Signed-off-by: Yannick Schulz <yschulz854@gmail.com> * removes unused compressor_ member from compresser writer class. Also delegates rest of the closing behavior to the base class in close method, as it is handled in the open and write methods of the compression writer Signed-off-by: Yannick Schulz <yschulz854@gmail.com> * Remove unrelated delta - message_definitions_ was intentionally allocated on the stack and should persist between writer close() and open() because it represents cache for message definitions which is not changes. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Don't call virtual methods from destructors Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Cleanup 'rosbag2_directory_next' after the test run Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Protect Writer::open(..) and Writer::close() with mutex on upper level - Rationale: To avoid race conditions if open(..) and close() could be ever be called from different threads. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Bugfix for WRITE_SPLIT callback not called for the last compressed file Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Bugfix for lost messages from cache when closing compression writer Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Use dedicated is_open_ flag instead of relying on storage_ Signed-off-by: Michael Orlov <michael.orlov@apex.ai> --------- Signed-off-by: Yannick Schulz <yschulz854@gmail.com> Signed-off-by: Michael Orlov <michael.orlov@apex.ai> Co-authored-by: Michael Orlov <michael.orlov@apex.ai> (cherry picked from commit 20cfc02) Co-authored-by: yschulz <yschulz854@gmail.com>

…backport #1599) (#1635) * re-applies fixes from #1590 to rolling. Also removes new message definition in sequential writer test for multiple open operations. Also clears topic_names_to_message_definitions_ and handles message_definitions_s underlying container similarly. Lastly, also avoids reset of factory in the compression writer, adds unit test there too. Signed-off-by: Yannick Schulz <yschulz854@gmail.com> Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * removes unused compressor_ member from compresser writer class. Also delegates rest of the closing behavior to the base class in close method, as it is handled in the open and write methods of the compression writer Signed-off-by: Yannick Schulz <yschulz854@gmail.com> * Remove unrelated delta - message_definitions_ was intentionally allocated on the stack and should persist between writer close() and open() because it represents cache for message definitions which is not changes. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Don't call virtual methods from destructors Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Cleanup 'rosbag2_directory_next' after the test run Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Protect Writer::open(..) and Writer::close() with mutex on upper level - Rationale: To avoid race conditions if open(..) and close() could be ever be called from different threads. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Bugfix for WRITE_SPLIT callback not called for the last compressed file Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Bugfix for lost messages from cache when closing compression writer Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Address build failure by using rcpputils::fs instead of std::filesystem - Note: On Iron we haven't migrated to the std::filesystem and using rcpputils::fs Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Adopt failing 'open_succeeds_twice' test for Iron Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Return from writer's open() immediately if storage already open Signed-off-by: Michael Orlov <michael.orlov@apex.ai> --------- Signed-off-by: Yannick Schulz <yschulz854@gmail.com> Signed-off-by: Michael Orlov <michael.orlov@apex.ai> Co-authored-by: Yannick Schulz <yschulz854@gmail.com>

…backport #1599) (#1635) * re-applies fixes from #1590 to rolling. Also removes new message definition in sequential writer test for multiple open operations. Also clears topic_names_to_message_definitions_ and handles message_definitions_s underlying container similarly. Lastly, also avoids reset of factory in the compression writer, adds unit test there too. Signed-off-by: Yannick Schulz <yschulz854@gmail.com> Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * removes unused compressor_ member from compresser writer class. Also delegates rest of the closing behavior to the base class in close method, as it is handled in the open and write methods of the compression writer Signed-off-by: Yannick Schulz <yschulz854@gmail.com> * Remove unrelated delta - message_definitions_ was intentionally allocated on the stack and should persist between writer close() and open() because it represents cache for message definitions which is not changes. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Don't call virtual methods from destructors Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Cleanup 'rosbag2_directory_next' after the test run Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Protect Writer::open(..) and Writer::close() with mutex on upper level - Rationale: To avoid race conditions if open(..) and close() could be ever be called from different threads. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Bugfix for WRITE_SPLIT callback not called for the last compressed file Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Bugfix for lost messages from cache when closing compression writer Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Address build failure by using rcpputils::fs instead of std::filesystem - Note: On Iron we haven't migrated to the std::filesystem and using rcpputils::fs Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Adopt failing 'open_succeeds_twice' test for Iron Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Return from writer's open() immediately if storage already open Signed-off-by: Michael Orlov <michael.orlov@apex.ai> --------- Signed-off-by: Yannick Schulz <yschulz854@gmail.com> Signed-off-by: Michael Orlov <michael.orlov@apex.ai> Co-authored-by: Yannick Schulz <yschulz854@gmail.com> (cherry picked from commit a360d9b) # Conflicts: # rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp # rosbag2_cpp/src/rosbag2_cpp/writer.cpp # rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp

… (backport #1599) (#1653) * [iron] Bugfix for writer not being able to open again after closing (backport #1599) (#1635) * re-applies fixes from #1590 to rolling. Also removes new message definition in sequential writer test for multiple open operations. Also clears topic_names_to_message_definitions_ and handles message_definitions_s underlying container similarly. Lastly, also avoids reset of factory in the compression writer, adds unit test there too. Signed-off-by: Yannick Schulz <yschulz854@gmail.com> Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * removes unused compressor_ member from compresser writer class. Also delegates rest of the closing behavior to the base class in close method, as it is handled in the open and write methods of the compression writer Signed-off-by: Yannick Schulz <yschulz854@gmail.com> * Remove unrelated delta - message_definitions_ was intentionally allocated on the stack and should persist between writer close() and open() because it represents cache for message definitions which is not changes. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Don't call virtual methods from destructors Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Cleanup 'rosbag2_directory_next' after the test run Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Protect Writer::open(..) and Writer::close() with mutex on upper level - Rationale: To avoid race conditions if open(..) and close() could be ever be called from different threads. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Bugfix for WRITE_SPLIT callback not called for the last compressed file Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Bugfix for lost messages from cache when closing compression writer Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Address build failure by using rcpputils::fs instead of std::filesystem - Note: On Iron we haven't migrated to the std::filesystem and using rcpputils::fs Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Adopt failing 'open_succeeds_twice' test for Iron Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Return from writer's open() immediately if storage already open Signed-off-by: Michael Orlov <michael.orlov@apex.ai> --------- Signed-off-by: Yannick Schulz <yschulz854@gmail.com> Signed-off-by: Michael Orlov <michael.orlov@apex.ai> Co-authored-by: Yannick Schulz <yschulz854@gmail.com> (cherry picked from commit a360d9b) # Conflicts: # rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp # rosbag2_cpp/src/rosbag2_cpp/writer.cpp # rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp * Address merge conflicts Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Fix for segfault in open_twice test - Ensure that writer_ is destructed before intercepted fake_metadata_ Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Fix for "open_succeeds_twice" test failure on second run - Use std::filesystem for temp files and folders operation. For some reason rcpputils::fs::delete_all(folder_name) wasn't able to delete temp folder with subfolders. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Adopt changes in TestRosbag2CPPAPI::minimal_writer_example for humble - The `serialized_msg2` is not owning the serialized data after the first call writer.write(serialized_msg2,..). i.e. need to use another message or another API in test. This is not a bug - this is by design. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> --------- Signed-off-by: Michael Orlov <michael.orlov@apex.ai> Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

yschulz marked this pull request as ready for review April 1, 2024 19:35

yschulz requested a review from a team as a code owner April 1, 2024 19:35

yschulz requested review from MichaelOrlov and james-rms and removed request for a team April 1, 2024 19:35

MichaelOrlov requested changes May 3, 2024

View reviewed changes

rosbag2_cpp/include/rosbag2_cpp/writers/sequential_writer.hpp Outdated Show resolved Hide resolved

rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp Outdated Show resolved Hide resolved

rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp Outdated Show resolved Hide resolved

MichaelOrlov changed the title ~~Fixes writer not being able to open again after closing~~ Bugfix for writer not being able to open again after closing May 6, 2024

MichaelOrlov approved these changes May 6, 2024

View reviewed changes

MichaelOrlov mentioned this pull request May 6, 2024

[iron] Bugfix for writer not being able to open again after closing (backport #1599) #1635

Merged

fujitatomoya approved these changes May 6, 2024

View reviewed changes

yschulz and others added 9 commits May 6, 2024 12:19

removes unused compressor_ member from compresser writer class. Also …

a967fdf

…delegates rest of the closing behavior to the base class in close method, as it is handled in the open and write methods of the compression writer Signed-off-by: Yannick Schulz <yschulz854@gmail.com>

Remove unrelated delta

98336d7

- message_definitions_ was intentionally allocated on the stack and should persist between writer close() and open() because it represents cache for message definitions which is not changes. Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

Don't call virtual methods from destructors

943d9b0

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

Cleanup 'rosbag2_directory_next' after the test run

db80280

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

Protect Writer::open(..) and Writer::close() with mutex on upper level

c19c1bc

- Rationale: To avoid race conditions if open(..) and close() could be ever be called from different threads. Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

Bugfix for WRITE_SPLIT callback not called for the last compressed file

1e1db8f

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

Bugfix for lost messages from cache when closing compression writer

5e3476c

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

Use dedicated is_open_ flag instead of relying on storage_

73a6993

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

MichaelOrlov force-pushed the fix_bag_not_closing_properly_rolling branch from 0bd5003 to 73a6993 Compare May 6, 2024 19:19

MichaelOrlov merged commit 20cfc02 into ros2:rolling May 6, 2024
14 checks passed

mergify bot mentioned this pull request May 6, 2024

[jazzy] Bugfix for writer not being able to open again after closing (backport #1599) #1639

Merged

mergify bot mentioned this pull request May 11, 2024

[humble] Bugfix for writer not being able to open again after closing (backport #1599) #1653

Merged

claraberendsen mentioned this pull request May 21, 2024

Buildfarmer log 2024/05/20 - 2024/05/27 osrf/buildfarm-tools#53

Closed

MichaelOrlov mentioned this pull request May 21, 2024

Fix for regression in open_succeeds_twice and minimal_writer_example tests #1667

Merged

This was referenced May 21, 2024

[iron] Fix for regression in open_succeeds_twice and minimal_writer_example tests (backport #1667) #1669

Merged

[jazzy] Fix for regression in open_succeeds_twice and minimal_writer_example tests (backport #1667) #1675

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix for writer not being able to open again after closing #1599

Bugfix for writer not being able to open again after closing #1599

yschulz commented Mar 30, 2024 •

edited by MichaelOrlov

yschulz commented Mar 30, 2024

fujitatomoya commented Apr 1, 2024

yschulz commented Apr 1, 2024

yschulz commented May 6, 2024

MichaelOrlov commented May 6, 2024

MichaelOrlov commented May 6, 2024

fujitatomoya commented May 6, 2024

MichaelOrlov commented May 6, 2024

MichaelOrlov commented May 6, 2024

clalancette commented May 6, 2024

fujitatomoya left a comment

MichaelOrlov commented May 6, 2024

MichaelOrlov commented May 6, 2024 •

edited

MichaelOrlov commented May 6, 2024

mergify bot commented May 6, 2024 •

edited

Bugfix for writer not being able to open again after closing #1599

Bugfix for writer not being able to open again after closing #1599

Conversation

yschulz commented Mar 30, 2024 • edited by MichaelOrlov

yschulz commented Mar 30, 2024

fujitatomoya commented Apr 1, 2024

yschulz commented Apr 1, 2024

yschulz commented May 6, 2024

MichaelOrlov commented May 6, 2024

MichaelOrlov commented May 6, 2024

fujitatomoya commented May 6, 2024

MichaelOrlov commented May 6, 2024

MichaelOrlov commented May 6, 2024

clalancette commented May 6, 2024

fujitatomoya left a comment

Choose a reason for hiding this comment

MichaelOrlov commented May 6, 2024

MichaelOrlov commented May 6, 2024 • edited

MichaelOrlov commented May 6, 2024

mergify bot commented May 6, 2024 • edited

✅ Backports have been created

yschulz commented Mar 30, 2024 •

edited by MichaelOrlov

MichaelOrlov commented May 6, 2024 •

edited

mergify bot commented May 6, 2024 •

edited