This is a proposal gathered from comments and ideas posted on the dev@ mailing list. The idea is to sanitize the plugin APIs, so that we have consistency in
- Return codes - There should be no "magic" values overloaded to PODs (other than NULL), and it should be easy to use.
- Error checking - We'll fail fast and early if a plugin uses the APIs incorrectly, or if the error is unrecoverable.
As such this proposal will mandate the following summary interface guidelines and contracts, to be used by all plugin APIs:
- An API which allocates memory will either return a good pointer, or not return at all. There are no such things as TS_ERROR_PTR.
- An API which returns a pointer to data is allowed to either return a good pointer, or a NULL pointer. No special pointers are ever returned.
- If the success or failure of an API can not be returned with a simple value (e.g. NULL pointer or not), then it should return TSReturnCode.
- TS_ERROR will never be used as a return value for anything returning an integer type. Meaning, only TSReturnCode can return TS_ERROR or TS_SUCCESS.
- Bad input (or output) parameters will always generate an ink_api_assert(), which by default will trigger on both release and debug builds.
The APIs will make no attempts to recover from memory allocation ("out of memory") errors, and will not communicate such errors back to the caller. Instead, we'll fail completely, early and fast, allowing for the server to restart and recover.
If an API can only return "good" data or not return at all, it's allowed to return PODs directly. This includes pointers, and in particular, NULL pointers. However, overloading semantics on e.g. integers to be returned with special values (e.g. "-5" means failure, while all other integers means length) is not allowed. In such cases, the API must return a TSReturnCode type to indicate success (TS_SUCCESS) or failure (TS_ERROR). The data is instead returned in an output parameter (handle) provided by the caller.
A NULL pointer doesn't indicate error but instead indicates if the API could satisfy the request or not. Such APIs should not use TSReturnCode. If however the API needs to communicate a failure, it should return TSReturnCode and return the pointer as an output parameter. This means that no special pointers should ever be returned, e.g. the usage of TS_ERROR_PTR is completely banned.
APIs that already have a return enum that includes "error conditions" should continue to use so. The one case I'm currently aware of is all the URL parser APIs, which return a TSParseResult. This enum communicates state in the parse process.
Bad input / output parameters
The idea here is that we'll change all existing sanity checks on input/output parameters in the SDK to become asserts. A plugin developer should not try to detect such coding errors, which is why it's imperative that we fail fast and early, with good errors (e.g. a stack trace). In the current code, these sanity checks are actually no-ops in release builds, and returns "errors" (of varying formats) in debug builds. This is very inconsistent, and illogical.
We'll also investigate further improvements, where the SDK can be enhanced to not only give a stack trace, but also tell (via syslog) that a particular plugin (by name) is poorly coded, and causes instability in the server.