Make plugin API cleanup functions noexcept - #13590
Open
bryancall wants to merge 2 commits into
Open
Conversation
Exceptions have never been a supported error channel across the plugin API; failures are reported through return values. This writes that contract down for the functions plugins commonly call during cleanup (destroy/free/release/close/cancel, the mutex family, and TSError): - Annotate 28 API functions noexcept in ts.h and their definitions. - Convert each definition to a function-try-block so an internal exception that would have escaped now aborts with a message naming the function, instead of terminating with no context inside a plugin destructor (destructors are implicitly noexcept). - Annotate ts::do_abort [[noreturn]] noexcept and contain formatting failures inside it and inside ts::shared_mutex::_call_fatal, whose message formatting allocates and could itself throw before the abort. - Update matching prototypes in the developer guide and state the no-exceptions contract in the plugin getting-started chapter.
Contributor
Author
|
[approve ci autest 2] |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR formalizes the “no exceptions across the plugin API” contract by marking commonly-used plugin cleanup/teardown entry points as noexcept and ensuring any unexpected exceptions become attributable aborts (instead of silent std::terminate, especially from destructors). It also updates developer documentation to reflect the contract.
Changes:
- Annotate cleanup/teardown TS API functions as
noexceptand wrap implementations with function-try-blocks that callink_abort("exception escaped %s", __func__). - Harden abort/fatal paths (
ts::do_abort,ts::shared_mutex::_call_fatal) so formatting/allocation failures cannot throw out ofnoexceptcontexts. - Update unit-test stubs and developer documentation to match the new
noexceptsignatures and describe the exception contract.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/tsutil/Assert.cc | Makes ts::do_abort noexcept and guards against formatting exceptions escaping. |
| src/iocore/cache/unit_tests/stub.cc | Updates stubbed TSIOBufferReaderConsume to match noexcept API signature. |
| src/cripts/unit_tests/stub.cc | Updates stubbed TS API functions (TSError, TSContData{Get,Set}, TSActionCancel, TSContDestroy) to noexcept. |
| src/api/InkIOCoreAPI.cc | Wraps selected IO-core TS API cleanup functions in noexcept + abort-on-exception try blocks. |
| src/api/InkAPI.cc | Wraps selected TS API cleanup/log functions in noexcept + abort-on-exception try blocks. |
| plugins/multiplexer/unit_tests/test_chunk_decoder.cc | Updates local TS API stubs (TSIOBufferReaderConsume, TSIOBufferReaderAvail) to noexcept. |
| plugins/header_rewrite/matcher_tests.cc | Updates local TS API stubs (TSError, TSHandleMLocRelease) to noexcept. |
| plugins/header_rewrite/header_rewrite_test.cc | Updates local TSError stub to noexcept. |
| plugins/experimental/sslheaders/unit_tests/test_sslheaders.cc | Updates local TSError stub to noexcept. |
| plugins/esi/test/print_funcs.cc | Updates local TSError stub to noexcept. |
| include/tsutil/TsSharedMutex.h | Makes _call_fatal [[noreturn]] noexcept and prevents formatting failures from throwing during abort paths. |
| include/tsutil/Assert.h | Declares ts::do_abort as [[noreturn]] noexcept. |
| include/tscpp/api/Logger.h | Updates forward declaration of TSError to include noexcept. |
| include/ts/ts.h | Adds noexcept to the selected plugin API cleanup/teardown function declarations. |
| doc/developer-guide/plugins/getting-started/index.en.rst | Documents the no-exceptions contract across the plugin API and the abort behavior for noexcept cleanup calls. |
| doc/developer-guide/api/functions/TSVConnShutdown.en.rst | Updates synopsis to noexcept. |
| doc/developer-guide/api/functions/TSVConnClose.en.rst | Updates synopsis to noexcept. |
| doc/developer-guide/api/functions/TSVConnAbort.en.rst | Updates synopsis to noexcept. |
| doc/developer-guide/api/functions/TSThreadDestroy.en.rst | Updates synopsis to noexcept. |
| doc/developer-guide/api/functions/TSTextLogObjectCreate.en.rst | Updates TSTextLogObjectFlush / TSTextLogObjectDestroy synopses to noexcept. |
| doc/developer-guide/api/functions/TSSslServerContextCreate.en.rst | Updates TSSslContextDestroy synopsis to noexcept. |
| doc/developer-guide/api/functions/TSMutexUnlock.en.rst | Updates synopsis to noexcept. |
| doc/developer-guide/api/functions/TSMutexLockTry.en.rst | Updates synopsis to noexcept. |
| doc/developer-guide/api/functions/TSMutexLock.en.rst | Updates synopsis to noexcept. |
| doc/developer-guide/api/functions/TSMutexDestroy.en.rst | Updates synopsis to noexcept. |
| doc/developer-guide/api/functions/TSMimeHdrDestroy.en.rst | Updates synopsis to noexcept. |
| doc/developer-guide/api/functions/TSMBufferCreate.en.rst | Updates TSMBufferDestroy / TSHandleMLocRelease synopses to noexcept. |
| doc/developer-guide/api/functions/TSIOBufferReader.en.rst | Updates TSIOBufferReader{Free,Consume,Avail} synopses to noexcept. |
| doc/developer-guide/api/functions/TSIOBufferCreate.en.rst | Updates TSIOBufferDestroy synopsis to noexcept. |
| doc/developer-guide/api/functions/TSHttpParserCreate.en.rst | Updates TSHttpParserDestroy synopsis to noexcept. |
| doc/developer-guide/api/functions/TSHttpHdrDestroy.en.rst | Updates synopsis to noexcept. |
| doc/developer-guide/api/functions/TSFetchCreate.en.rst | Updates TSFetchDestroy synopsis to noexcept. |
| doc/developer-guide/api/functions/TSDebug.en.rst | Updates TSError synopsis to noexcept. |
| doc/developer-guide/api/functions/TSContDestroy.en.rst | Updates synopsis to noexcept. |
| doc/developer-guide/api/functions/TSContDataSet.en.rst | Updates synopsis to noexcept. |
| doc/developer-guide/api/functions/TSContDataGet.en.rst | Updates synopsis to noexcept. |
| doc/developer-guide/api/functions/TSActionCancel.en.rst | Updates synopsis to noexcept. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
noexceptints.h, the cleanup/teardown family that destructors call:TSContDestroy,TSHandleMLocRelease,TSIOBuffer*/TSMBufferDestroy,TSHttpHdrDestroy,TSMimeHdrDestroy,TSVConn{Close,Abort,Shutdown},TSActionCancel,TSCacheKeyDestroy,TSSslContextDestroy,TSFetchDestroy,TSTextLogObject*, theTSMutex*family,TSThread{Wait,Destroy},TSContData{Get,Set}, andTSError).ink_abort("exception escaped %s", __func__). An exception that would previously have escaped now produces a named, attributable abort instead of a barestd::terminateinside a plugin destructor.ts::do_abort[[noreturn]] noexceptand contains formatting failures inside it, and does the same forts::shared_mutex::_call_fatal. Both build their message withswoc::bwprint/Strerror, which allocate, so the abort path could itself throwbad_allocbefore reaching the abort, from inside~write_guard/~read_guard.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.ccbuilt before and after with identical flags:__textgrew 896 bytes (+0.66%, about 45 bytes per wrapped function) and__gcc_except_tabgrew 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
noexceptno longer need their own landing pads around these calls, so plugin cleanup code should get marginally smaller.Coverity
This should resolve 83 Coverity
UNCAUGHT_EXCEPTdefects.The checker flags this pattern because it has to assume any call in a destructor can throw. I arrived at that number by parsing the body of every destructor it flags and classifying what each one calls: 80 whose only calls are functions this PR makes
noexcept, plus 3ts::shared_mutexguard destructors covered by the_call_fatalfix. By area that is 61 in plugins, 13 intscpp/api, 5 in core, 2 in examples, and 2 in test code.Treat 83 as a projection rather than a measurement. It assumes the checker credits a
noexceptcallee and stops reporting the caller, which is the documented behavior but is not something I can confirm without a scan of the merged tree. The first analysis run after this lands will give the real figure, and I will follow up with it.Destructors whose throw path is a member subobject rather than an API call are not affected by this change and need their own root cause, as do the CLI tool entry points. Those are follow-up work.
ABI
Adding
noexceptdoes not change the ABI. In the Itanium C++ ABI an exception specification is part of a function type but is not encoded in the mangled name of a function declaration, and it does not affect the calling convention.I verified this rather than assuming it. Building
src/api/InkAPI.ccbefore and after with identical flags, both objects export 748 text symbols with zero name differences:TSContDestroyis_Z13TSContDestroyP10tsapi_contin both,TSErroris_Z7TSErrorPKczin both. As an end-to-end check, a caller compiled against the old declaration and linked against the newnoexceptdefinition resolves to the same symbol, links, and runs. So an existing plugin binary keeps working against a rebuiltlibtsapiand vice versa.Function pointers are also unaffected: a
noexceptfunction converts implicitly to a plain function-pointer type, sovoid (*p)(TSCont) = TSContDestroy;still compiles. Mangling would only shift if a parameter or template argument were spelled as anoexceptfunction type, which nothing in the API does. Nothing in the tree takes the address of any annotated function.The compatibility cost is at the source level, not the binary level, and is described below.
Notes for reviewers
catchexpecting 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. It is a real change, though, and worth a release note.noexcepthas been part of the function type since C++17, so any translation unit that includests.hand separately declares one of these functions withoutnoexceptbecomes ill-formed. This applies equally to a plugin built as C++17 or C++20. 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.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.