Core: Avoid uncaught exceptions while loading devices. - #977
Conversation
Adds two catch blocks after the existing catch (const CMMError&): - catch (const std::exception&) — logs typeid(e).name() and e.what(), unloads devices, rethrows as CMMError with the type and message embedded. - catch (...) — same handling for non-standard exceptions, without detail since none is available. Both mirror the existing CMMError path: clear isLoadingSystemConfiguration_, unload all devices, then propagate. The nested unload calls have their own catches so a secondary failure during cleanup can't escape. The payoff is that CMMError has a SWIG typemap, so these surface as normal Java exceptions with a readable message instead of killing the JVM. The ordering is load-bearing. catch (const CMMError&) must stay first. CMMError doesn't derive from std::exception in this codebase — I checked the SWIG typemaps treat them separately — but even so, ordering derived-to-base is what keeps the existing behavior exactly intact. As written, CMMError still takes the original path untouched. typeid(e).name() is MSVC-specific in its output. On MSVC you get readable text like class std::out_of_range; on GCC/Clang it's mangled (St12out_of_range). It's still diagnostic on every platform, just less pretty on non-Windows. If that bothers you, dropping the type and keeping only what() loses little. This is a behavior change, not purely additive. Previously a std::exception from an adapter crashed the process; now it unloads all devices and throws. That's clearly better, but it means configs that used to hard-crash will now fail gracefully — worth knowing if any downstream code somehow depends on the old behavior (very unlikely, but this is Core). Scope. This covers loadSystemConfiguration only. The same gap exists on other JNI-reachable Core entry points — initializeDevice, setProperty, and so on. Given the branch name, you may want to consider whether the fix belongs at a broader boundary rather than this one function. I'd not expand it without your call.
There was a problem hiding this comment.
Pull request overview
This PR hardens CMMCore::loadSystemConfiguration() against non-CMMError C++ exceptions so they don’t cross the JNI boundary uncaught (which can terminate the JVM), by translating them into CMMError after logging and cleanup.
Changes:
- Add
catch (const std::exception&)to log exception details, unload devices, and throw aCMMErrorwith embedded type/message. - Add
catch (...)to handle non-standard exceptions similarly (without details). - Include
<typeinfo>to supporttypeid(e).name()logging.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try | ||
| { | ||
| unloadAllDevices(); | ||
| } | ||
| catch (const CMMError& err) | ||
| { | ||
| LOG_ERROR(coreLogger_) << | ||
| "Error occurred while unloading all devices: " << | ||
| err.getFullMsg(); | ||
| } | ||
| catch (const std::exception& err) | ||
| { | ||
| LOG_ERROR(coreLogger_) << | ||
| "Unhandled C++ exception while unloading all devices: " << | ||
| err.what(); | ||
| } |
| catch (const std::exception& e) | ||
| { | ||
| // A device adapter (or other code below us) threw a C++ standard | ||
| // exception. There is no SWIG typemap for these, so allowing one to | ||
| // escape through the JNI boundary terminates the whole JVM with an | ||
| // EXCEPTION_UNCAUGHT_CXX_EXCEPTION hs_err file and no usable | ||
| // diagnostic. Log what we know and translate to CMMError, which does | ||
| // have a typemap and so surfaces as a normal error to the application. | ||
| isLoadingSystemConfiguration_ = false; |
I'm sorry but I disagree: terminating with the correct message ("uncaught C++ exception") is much better. Throwing from a device adapter is a severe programming error (similar to dereferencing an invalid pointer), and we should not convert that to a recoverable run-time error (any more than we should continue running after a segfault). This is especially true because device adapter code may not be written to clean up correctly on the unintentional throw path. Pretending that we can recover from a buggy state just makes troubleshooting that much harder. It also conveys the wrong message to device adapter developers: they may think it's okay to throw Also, catching exceptions requires a higher level of compatibility between the throwing and catching DLLs than the MMDevice binary interface usually requires. In particular, standard library classes like Handling the exception also means that we no longer get a stack trace that indicates where the illicit exception came from, and also that we cannot attach a debugger after a crash (in the way Windows normally allows if you have Visual Studio installed). In summary, better to fail fast and accurately on a critical bug like throwing from a device adapter. I know it's worse for the user on the surface, but obscuring the bug to developers doesn't help anybody either. (I'm aware of some existing |
Adds two catch blocks after the existing catch (const CMMError&):
Both mirror the existing CMMError path: clear isLoadingSystemConfiguration_, unload all devices, then propagate. The nested unload calls have their own catches so a secondary failure during cleanup can't escape. The payoff is that CMMError has a SWIG typemap, so these surface as normal Java exceptions with a readable message instead of killing the JVM.
The ordering is load-bearing. catch (const CMMError&) must stay first. CMMError doesn't derive from std::exception in this codebase — I checked the SWIG typemaps treat them separately — but even so, ordering derived-to-base is what keeps the existing behavior exactly intact. As written, CMMError still takes the original path untouched.
typeid(e).name() is MSVC-specific in its output. On MSVC you get readable text like class std::out_of_range; on GCC/Clang it's mangled (St12out_of_range). It's still diagnostic on every platform, just less pretty on non-Windows. If that bothers you, dropping the type and keeping only what() loses little.
This is a behavior change, not purely additive. Previously a std::exception from an adapter crashed the process; now it unloads all devices and throws. That's clearly better, but it means configs that used to hard-crash will now fail gracefully.
Scope. This covers loadSystemConfiguration only. The same gap exists on other JNI-reachable Core entry points — initializeDevice, setProperty, and so on.