bryancall opened a new pull request, #13688: URL: https://github.com/apache/trafficserver/pull/13688
Coverity reports uncaught-exception escapes from test destructors and test `main()` functions. Part of #13682. ### The one thing to decide **`src/iocore/eventsystem/unit_tests/test_MIOBufferWriter.cc` redefines `_ink_assert` to `throw InkAssertExcept()`.** It already carries its own `// coverity[UNCAUGHT_EXCEPT:FALSE]` noting that "Coverity is confused and thinks this `_ink_assert` is the one used in traffic_server." Coverity merges symbol definitions across the whole analysis, so *every* `ink_assert()` in the tree resolves to that throwing definition. That is why these destructors look like they can throw. They reach an assert one of three ways: - directly (`~CacheTestSM`) - via `THREAD_FREE()` → `thread_freeup()`, which asserts on its postcondition (`~EasyURL` through `HdrHeap::destroy()`; `~CacheReadTest` / `~CacheWriteTest` through `free_MIOBuffer` → `dealloc_reader`) - via `EThread::schedule()` (`~TestContChain`) The aborting `_ink_assert` from `unit_tests/stub.cc` is what actually links into these binaries, so the exception cannot occur. ### Important for anyone fixing this family **Adding `noexcept` to a destructor is a no-op.** A user-provided destructor with no exception-specification is already implicitly `noexcept(true)` in C++20, even when its body calls a potentially-throwing function (`std::is_nothrow_destructible_v` is 1). An explicit `noexcept` changes nothing and cannot clear the finding. ### Two treatments, chosen per site **Suppression** where the exception provably cannot occur and there is nothing to handle — `// coverity[UNCAUGHT_EXCEPT:FALSE]` with the trace in a comment: CIDs 1528624, 1528646, 1528704, 1528771, 1591506. **`try`/`catch` + `ink_abort`** where a guard is strictly better than a silent `std::terminate`, so a genuine escape still fails loudly with context: CIDs 1518135, 1559190 (`~ParentTest`), 1686062 (`~HoldOnEThread`), 1518878, 1528569 (`~NetVCTest`). `ci/coverity-model.cpp` already teaches Coverity that `ink_abort` panics. **`try`/`catch` returning a non-zero exit status** for the `main()` findings, so a test failure is still reported rather than terminating: CIDs 1523670, 1528654, 1644283 (`unit_test_main.cc`), 1523686, 1528590, 1644327 (`test_AIO.cc`), 1686026, 1587256, 1587267 (`test_RefCountCache.cc`), 1528601 (`test_HPACK.cc`). `main` is deliberately **not** marked `noexcept`. Several commits close three CIDs, because Coverity reports one per throwing call site and a single handler covers them all. ### Alternative worth considering The project also has precedent for `// coverity[exn_spec_violation]` annotations (`Stripe::~Stripe()`, `HttpSM::~HttpSM()`). If a reviewer prefers one uniform treatment across the whole family rather than suppression-where-provable and guard-where-useful, the destructor commits are the ones to swap. ### Verification `test_records`, `test_tsutil`, `test_proxy_hdrs`, `test_proxy_hdrs_xpack`, `test_cache`, `test_hostdb`, `test_tscore`, `test_tsconfig` — 321 tests, all passing. Every touched file compiles independently on this branch. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
