Check existence of a file before passing it to the mcap reader #1594
Conversation
…mcap::FileStreamReader in order to avoid assertion failing Signed-off-by: Christopher Wecht <cwecht@mailbox.org>
| @@ -334,6 +335,9 @@ void MCAPStorage::open_impl(const std::string & uri, const std::string & preset_ | |||
| case rosbag2_storage::storage_interfaces::IOFlag::READ_ONLY: { | |||
| relative_path_ = uri; | |||
| input_ = std::make_unique<std::ifstream>(relative_path_, std::ios::binary); | |||
| if (!input_->is_open()) { | |||
| throw std::runtime_error(std::strerror(errno)); | |||
There was a problem hiding this comment.
if input_ failed to open the file during construction, the errno is set accordingly? i was expecting we need to check the failbit instead, but it would be better that we can just use errno!
How about adding more information for runtime error exception?
| throw std::runtime_error(std::strerror(errno)); | |
| throw std::runtime_error("Failed to open storage " + relative_path_ + std::strerror(errno)); |
There was a problem hiding this comment.
- std::ifstream and errno is a bit complicated: cppreference (hence the C++ standard) don't mention errno at all. However, std::ifstream is usually implemented in terms of fopen/open which in turn is specified to set errno. I think, it is reasonable to realy on this behavior. An alternative could be to set the execption mask of
input_. IMO this has the disadvantage that we would need to disable it after that probably. Otherwise there might occour other exceptions inFileStreamReader, which might not be expected or not dealt with properly. - Regarding the exception message: this exception will be catched here: https://github.com/ros2/rosbag2/blob/rolling/rosbag2_storage/src/rosbag2_storage/impl/storage_factory_impl.hpp#L135 Here, the path is logged already, so there is no need to duplicate this infromation in the exception message.
There was a problem hiding this comment.
thanks for the explanation! lgtm!
MichaelOrlov
changed the title
|
Gist: https://gist.githubusercontent.com/MichaelOrlov/6dee8d20cb96a4d63b417bf6df46932e/raw/624b11e64730f3df462ee2425c8fcd8c6ca7fa78/ros2.repos
|
|
@cwecht There is one new warning on Windows CI build
Could you please address it by using |
Signed-off-by: Christopher Wecht <cwecht@mailbox.org>
Turns out there is already |
|
@cwecht change looks good, CI just starts. 🤞
|
|
https://build.ros2.org/job/Rpr__rosbag2__ubuntu_noble_amd64/62/console failing seems to be caused by some kind of hicup/issue on the toolchains/buildservers side, right? In case we need that build to successes, I'd suggest to rerun it. |
|
@fujitatomoya @MichaelOrlov how do we proceed? |
|
@cwecht The buildfarm CI is green we are good to go. Never mind. |
…mcap::FileStreamReader in order to avoid this assertion: failing https://github.com/foxglove/mcap/blob/9d73125e50121c1eeb2ceb9d13db51ff923b0420/cpp/mcap/include/mcap/reader.inl#L90
This is an issue if you happen to run rosbag build in debug mode (in which assertions are enabled). As it is the behavior of debug and release mode is inconsistent as in debug mode the assertion is raised and in release mode an exception is raised.