Make mip_node_t destructor exception-safe (SonarQube CRIT BUG cpp:S1048)#1230
Make mip_node_t destructor exception-safe (SonarQube CRIT BUG cpp:S1048)#1230rgsl888prabhu wants to merge 6 commits into
Conversation
The iterative teardown uses `std::vector::push_back`, which can throw `std::bad_alloc`. Letting that propagate out of a destructor calls `std::terminate`. Wrap the body in a try/catch(...) so the destructor stays exception-free. Under OOM the catch absorbs the failure, and any descendants still attached to `children` are destroyed via the recursive unique_ptr chain as this frame unwinds — which risks stack overflow on a very deep tree, but only when memory is already exhausted, and is strictly better than `std::terminate`. Clears 2 CRITICAL bug findings on `main`: - cpp/src/branch_and_bound/mip_node.hpp:49 - cpp/src/branch_and_bound/mip_node.hpp:54 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThree destructors were made exception-tolerant: mip_node_t’s breadth-first descendant detachment, timing_raii_t’s timing-sample push, and grpc_client_t’s shutdown/join sequence now swallow exceptions so destructors do not throw. ChangesDestructor exception-safety
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
rgsl888prabhu
added
non-breaking
|
Do we intent to fail nicely on every potential line that could throw |
Codebase survey for other destructors that can throw bad_alloc found the same pattern in fj_cpu.cu's `timing_raii_t::~timing_raii_t` (push_back into a std::vector<double>). SonarQube missed it because .cu files aren't covered by its C++ analyzer; addresses the broader concern from the PR review. Same try/catch(...) approach. Losing one timing sample under OOM is acceptable; the alternative is std::terminate. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
stop_log_streaming() can throw: std::thread::join may throw std::system_error if the join fails, and std::lock_guard's mutex acquisition can throw on a mutex error. Wrap the call in try/catch(...) so the destructor stays exception-free. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Good question — I shouldn't have framed this as "fail nicely on bad_alloc." The narrower motivation is the destructor contract specifically: since C++11, destructors are implicitly I also surveyed
SonarQube only flagged |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6e1fa19c-ebee-4801-9e5f-7248286561d0
📒 Files selected for processing (1)
cpp/src/grpc/client/grpc_client.cpp
mlubin
requested review from
Kh4ster and
nguidotti
and removed request for
mlubin
|
I'll defer approval to others who have more familiarity with this code. |
The previous version wrapped stop_log_streaming() in try/catch in the
destructor, but that does not actually prevent std::terminate:
1. If lock_guard or TryCancel throw before the swap, log_thread_
still owns a joinable std::thread; when the surrounding
grpc_client_t destructor finishes and log_thread_ is destroyed,
a joinable thread's destructor calls std::terminate — outside the
catch block.
2. If t->join() throws, the local unique_ptr<thread> t is destroyed
during the same unwind. Same problem — joinable thread destructor
terminates before the catch can fire.
Inline a noexcept shutdown in the destructor instead: cancel under a
try/catch, swap log_thread_ into a local (so the member is now empty),
join the local, and on join failure detach the thread before its
destructor runs. The public stop_log_streaming() keeps its existing
contract for the solve_lp / solve_mip callers.
Thanks to CodeRabbit for the catch.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rgsl888prabhu
requested review from
hlinsen and
rg20
and removed request for
kaatish
|
Looks like you are catching the exceptions but not reporting them. You can use cuopt_expects and throw RunTimeError. |
…catches Per review on PR #1230: silently catching in destructors hides real failures. Switch each catch block to log via CUOPT_LOG_ERROR with the exception's what(), matching the pattern used in solve.cu. The catch (...) is kept as a noexcept safety net. (Cannot literally throw RuntimeError as the review suggested — that would re-introduce the std::terminate path this PR exists to close. Logging is the closest we can get while satisfying the destructor noexcept contract.) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@rg20 Good point — silently swallowing hides real failures. Switched each catch in this PR to log via One thing I could not literally do: throw |
3 participants
Summary
The iterative-teardown destructor in
mip_node_tusesstd::vector::push_backto walk and detach the subtree.push_backcan throwstd::bad_alloc, and a destructor that lets an exception propagate callsstd::terminate.Wraps the body in
try { ... } catch (...) {}so the destructor stays exception-free. Under OOM the catch absorbs the failure, and any descendants still attached tochildrenare destroyed via the recursiveunique_ptrchain as this frame unwinds — which risks stack overflow on a very deep tree under OOM, but is strictly better thanstd::terminate.