bryancall opened a new pull request, #13590:
URL: https://github.com/apache/trafficserver/pull/13590
Exceptions have never been a supported error channel across the plugin API:
API functions report failure through return values, and no header or document
has ever promised that a TS API call throws anything a plugin could catch. That
contract was never written down, though, so any exception escaping from the
core into plugin code is both undiagnosable and invisible to static analysis.
It matters most in destructors. Plugin destructors routinely call the API
cleanup functions, destructors are implicitly `noexcept`, and so an exception
escaping one of those calls terminates the process with no attribution at all.
### What this does
- Annotates 28 API functions `noexcept` in `ts.h` — the cleanup/teardown
family that destructors call (`TSContDestroy`, `TSHandleMLocRelease`,
`TSIOBuffer*`/`TSMBufferDestroy`, `TSHttpHdrDestroy`, `TSMimeHdrDestroy`,
`TSVConn{Close,Abort,Shutdown}`, `TSActionCancel`, `TSCacheKeyDestroy`,
`TSSslContextDestroy`, `TSFetchDestroy`, `TSTextLogObject*`, the `TSMutex*`
family, `TSThread{Wait,Destroy}`, `TSContData{Get,Set}`, and `TSError`).
- Converts each definition to a function-try-block whose handler calls
`ink_abort("exception escaped %s", __func__)`. An exception that would
previously have escaped now produces a named, attributable abort instead of a
bare `std::terminate` inside a plugin destructor.
- Annotates `ts::do_abort` `[[noreturn]] noexcept` and contains formatting
failures inside it, and does the same for `ts::shared_mutex::_call_fatal`. Both
build their message with `swoc::bwprint`/`Strerror`, which allocate, so the
abort path could itself throw `bad_alloc` before reaching the abort — from
inside `~write_guard`/`~read_guard`.
- Updates the matching prototypes in the developer guide (22 pages) and
states the no-exceptions contract in the plugin getting-started chapter.
### Overhead
None on the happy path, as expected for table-driven exception handling.
Comparing generated code for a representative wrapped function against the same
function unwrapped, the instruction sequences are identical except that the
final call can no longer be a tail call. Measured on `InkAPI.cc` built before
and after with identical flags: `__text` grew 896 bytes (+0.66%, about 45 bytes
per wrapped function) and `__gcc_except_tab` grew 500 bytes of read-only data
that is never touched unless an exception unwinds.
In the other direction this is a small optimization enabler: callers that
can see `noexcept` no longer need their own landing pads around these calls, so
plugin cleanup code should get marginally smaller.
### Coverity
`UNCAUGHT_EXCEPT` is the single largest class of outstanding defects on the
[Coverity Scan
project](https://scan.coverity.com/projects/apache-traffic-server): 203 of 538
in the High Impact Outstanding view (build master-20260826).
Parsing the body of every flagged destructor, 83 findings are directly
attributable to this change — 80 destructors whose only calls are functions
this PR makes `noexcept`, plus 3 `TsSharedMutex` guard destructors covered by
the `_call_fatal` fix. By area: 61 in plugins, 13 in `tscpp/api`, 5 in core, 2
in examples, 2 in test code. Representative CIDs: 1528663, 1528712, 1660590,
1660588, 1533104, 1528778.
The other 120 need separate root causes (the `main()` entry points in the
CLI tools, and destructors whose throw path is a member subobject rather than
an API call); those are follow-up changes, not this one.
### Notes for reviewers
- **Behavior change:** a plugin that wrapped a TS API call in a `catch`
expecting to catch a core-internal exception would now see an abort. Exceptions
were never a supported error channel here, so this standardizes existing
practice rather than removing a documented one — but it is a real change and
worth a release note.
- **Source compatibility:** since C++17 `noexcept` is part of the function
type, so any translation unit that includes `ts.h` *and* separately declares
one of these functions without `noexcept` becomes ill-formed. Seven in-tree
test stubs that redefine API functions needed the annotation added; out-of-tree
plugins doing the same will need the same one-word change. The stub edits are
mechanical and have to land in the same commit or the build breaks, which is
why they are here rather than split out.
- **Selection rule:** the set is "the cleanup/teardown entry points a
destructor can reasonably call," derived by parsing what the flagged
destructors actually call, then extended to the obvious siblings
(`TSMutexDestroy`, `TSMutexLockTry`, `TSMimeHdrDestroy`, `TSSslContextDestroy`)
so the boundary is not an artifact of a scanner hit list. Widening it to the
whole API surface is a reasonable follow-up if reviewers prefer that; it did
not seem like the right first step.
--
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]
