Catch other exceptions from readDataset to handle file open failures #124
Conversation
…en it fails to open a dataset Signed-off-by: Akshita Mavurapu <akshitamavurapu@gmail.com>
|
@akshitamav Sorry it took me so long to get to this. Your branch inherited temporarily broken Python tests that we in the master branch, so the tests aren't currently passing. This has since been fixed in the master branch. Can you merge the latest OpenNavigationSurface:master into your branch and re-push? Thanks! |
@akshitamav Can you merge the latest OpenNavigationSurface:master into your branch and re-push? |
Update branch
|
I have updated my branch, I think it should be right now |
|
I'm not really sure why this is failing here |
The Test Reporting fails currently for pull requests since they run with read-only credentials. I thought I had fixed this last week to skip the parts that would publish the results (which requires write credentials), but apparently it needs more work. We can ignore for now. |
|
@akshitamav Do you want to resolve the issue you identified here in this PR? |
|
Sure! I'm working on that right now. I'm not sure I can make it work the same way on windows (microsoft makes it work differently there) but I'm going to make it so that it also calls the "old" signal handler too so that it doesn't break other libraries signal handlers. Do we need it to work on windows as well? |
Signed-off-by: Akshita Mavurapu <akshitamavurapu@gmail.com>
|
I added my code so far into the branch now @selimnairb, but I need to do some more testing to make sure it works. It lets people set cleanup functions, and also calls any handlers that were already set before exiting so it doesn't interfere with other libraries |
@akshitamav This looks like an interesting start and would cover opening BAG files using the C API (which is in the code, but not well tested currently; the preference is for people to use the C++ API). I think we would also need to add this to bagCreateFromFile, bagCreateFromBuffer. Is it necessary to have Also, do we need to add |
|
Ah, yeah, we should add more construction of them. I actually made it so that it unsets itself when it destructs specifically so that code that isn't bag doesn't get affected by it. It is like "take out what you take in" for camping, so someone shouldn't get the bag handler run when their other unrelated code is running. When it returns from BAG code, it restores the original signal handlers. I need to add it to more places to be thorough, but we should probably only add it to places where HDF5 or libxml code can run so that it isn't being used more than it needs to be. |
Signed-off-by: Akshita Mavurapu <akshitamavurapu@gmail.com>
Signed-off-by: Akshita Mavurapu <akshitamavurapu@gmail.com>
Signed-off-by: Akshita Mavurapu <akshitamavurapu@gmail.com>
|
@selimnairb could you try starting the workflows to test the code? I think this should make it compile on windows now. |
|
@selimnairb could you try starting the workflows to test the code? |
Signed-off-by: Akshita Mavurapu <akshitamavurapu@gmail.com>
Signed-off-by: Akshita Mavurapu <akshitamavurapu@gmail.com>
Signed-off-by: Akshita Mavurapu <akshitamavurapu@gmail.com>
Signed-off-by: Akshita Mavurapu <akshitamavurapu@gmail.com>
Signed-off-by: Akshita Mavurapu <akshitamavurapu@gmail.com>
| // if the handlers are the same, don't call "bag handler" twice | ||
| char* signal_name; | ||
|
|
||
| #define TERM_STRING "a critical error occurred within BAG or a dependency (probably libxml or HDF5), exiting to prevent corruption or unexpected behavior\n"\ |
There was a problem hiding this comment.
probably shouldn't put any newlines in this string since we don't know where it's going to ultimately be written to.
| } | ||
|
|
||
| void call_cleanup_functions(int signal) { | ||
| //std::cout << "calling cleanup functions" << std::endl; |
There was a problem hiding this comment.
Remove commented out code.
|
|
||
| // if there was a handler before this, that isn't the BAG handler, | ||
| // call that one | ||
|
|
| this->call_previous_handler(signal); | ||
|
|
||
| // if nothing has exited in the previous handlers, exit here | ||
| //std::cout << "exiting" << std::endl; |
There was a problem hiding this comment.
remove commented out code
|
|
||
| struct BagAbortHook { | ||
| BagAbortHook* previous; | ||
| //void (*old_handler[255])(int); |
There was a problem hiding this comment.
Remove commented out code
| BagAbortHook* previous; | ||
| //void (*old_handler[255])(int); | ||
| //void (*current_handler[255])(int); | ||
| struct sigaction old_handlers[255]; |
There was a problem hiding this comment.
The size of these handler arrays should be a constant defined by a macro (i.e. #define somewhere).
| std::cout << "made hook" << std::endl; | ||
| // set these all to null so that when restoring them | ||
| // we know which ones were added | ||
| for (int i = 0; i < 255; i++) { |
There was a problem hiding this comment.
use a uint8_t for looks like this?
There was a problem hiding this comment.
Please integrate this into the C++ test suite so that it is automatically run in CI.
readDataset throws (from HDF5) many exceptions that inherit from H5::Exception.
These exceptions should be handled and printed as a failure to open the dataset, so BAG can exit gracefully.