From d6cc5d373c2cd4581c19506fb24905841b4bde3f Mon Sep 17 00:00:00 2001 From: Francesco Chemolli <5175948+kinkie@users.noreply.github.com> Date: Sun, 21 Jun 2026 21:48:24 +0100 Subject: [PATCH 01/12] Add section on error reporting to coding guidelines ---- Cherry-picked commit 2711584f7325ec71ca44400b41f8278ed2a0b1de from Francesco's coding-guidelines-error-reporting branch (PR 48) --- docs/DeveloperResources/SquidCodingGuidelines.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/DeveloperResources/SquidCodingGuidelines.md b/docs/DeveloperResources/SquidCodingGuidelines.md index d217c2e9..364d2dfc 100644 --- a/docs/DeveloperResources/SquidCodingGuidelines.md +++ b/docs/DeveloperResources/SquidCodingGuidelines.md @@ -324,6 +324,12 @@ components at build time. * MUST be used inside .h to wrap relevant code. +## Error reporting + +To report an error and abort the current transaction, throw a `TextException("descriptive text", Here())`. +Use `Assure(condition)` to test an invariant and abort the current transaction. +To check system-leve invariants, where a failure needs to terminate Squid, use `xassert(condition)` + ## See Also * [DeveloperResources](/DeveloperResources). From c5402169bb4889ef7d5a6bf7976318f850277fee Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Thu, 2 Jul 2026 15:47:54 -0400 Subject: [PATCH 02/12] fixup: Typo ---- Applied changes suggested by Amos for Francesco's coding-guidelines-error-reporting branch (PR 48). --- docs/DeveloperResources/SquidCodingGuidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/DeveloperResources/SquidCodingGuidelines.md b/docs/DeveloperResources/SquidCodingGuidelines.md index 364d2dfc..a28ba54f 100644 --- a/docs/DeveloperResources/SquidCodingGuidelines.md +++ b/docs/DeveloperResources/SquidCodingGuidelines.md @@ -328,7 +328,7 @@ components at build time. To report an error and abort the current transaction, throw a `TextException("descriptive text", Here())`. Use `Assure(condition)` to test an invariant and abort the current transaction. -To check system-leve invariants, where a failure needs to terminate Squid, use `xassert(condition)` +To check system-level invariants, where a failure needs to terminate Squid, use `xassert(condition)` ## See Also From f4bdbf8743864bb1c127bfd6f38b01b448c3e4fd Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Thu, 2 Jul 2026 17:23:16 -0400 Subject: [PATCH 03/12] Initial unpolished rewrite Inspired by github.com/measurement-factory/squid-notes/blob/start/errors.md --- .../SquidCodingGuidelines.md | 93 ++++++++++++++++++- 1 file changed, 89 insertions(+), 4 deletions(-) diff --git a/docs/DeveloperResources/SquidCodingGuidelines.md b/docs/DeveloperResources/SquidCodingGuidelines.md index a28ba54f..30976f0e 100644 --- a/docs/DeveloperResources/SquidCodingGuidelines.md +++ b/docs/DeveloperResources/SquidCodingGuidelines.md @@ -324,11 +324,96 @@ components at build time. * MUST be used inside .h to wrap relevant code. -## Error reporting -To report an error and abort the current transaction, throw a `TextException("descriptive text", Here())`. -Use `Assure(condition)` to test an invariant and abort the current transaction. -To check system-level invariants, where a failure needs to terminate Squid, use `xassert(condition)` +## Error handling + +There are several primary ways to handle various error conditions in Squid +code. For any given context, only one is usually the correct choice. Using the +list below, pick the _first_ one that matches your use case. See further below +for notes about optional custom assertion messages and legacy code. + +1. If the condition can be checked at compilation time, use `static_assert()`. + Minor code adjustments to make compile-time assertions possible may be + allowed, but Squid currently avoids explicit `constexpr`, and sprinkling + Squid code with many `constexpr` specifiers to get some compile-time + assertion working is usually a bad idea. + +2. If the condition describes a code invariant (e.g., "our caller must supply + a non-nil pointer"), and the bug violating that invariant is likely to + affect transactions that do not check this condition (e.g., a cache may get + corrupted, feeding other/independent transactions bogus response data), use + `assert(3)`. The guarantees provided by `assert()` are significant, but + they do not automatically extend beyond a single kid process. There is + currently no mechanism that is guaranteed to terminate the entire SMP Squid + instance. + +3. If the condition describes a code invariant (e.g., "our caller must supply + a non-nil pointer"), and the bug violating that invariant is likely to + affect just the transaction checking it, use `Assure()`. In most cases, + `Assure()` failures kill the checking transaction but keep its kid process + alive. Neither outcome is guaranteed though because Squid may catch and + handle the exception before it kills the transaction or the exception may + propagate to the top level where it kills the kid process. + +4. If the condition describes some input characteristics (e.g., "the client + sent a syntactically valid HTTP request to Squid"), do not use any of the + above calls. Instead, create and throw a `TextException` object, return + `std::nullopt`, or otherwise signal the problem to the caller. In most + cases, adding a level-0/1 `debugs()` message is not a good idea. This is + especially true when Squid cache administrator can do nothing about that + bad input, and that bad input does not represent some very unusual or + dangerous situation. Most input validation failures ought to be reflected + in various error details logged to `access.log`, not level-0/1 `cache.log` + messages. + + +### Squid bug workarounds + +In special rare cases, the correct choice is `Assure()`, but it is possible to +implementing a temporary workaround that would be much better than killing the +transaction. In this case, it may be permissible to emulate `Assure()` +reporting without throwing an exception: + +```C++ +debugs(54, DBG_IMPORTANT, "ERROR: Squid BUG: wrong fd_note ID: " << fdNoteId); +``` + +Do a `git grep ERROR:.Squid.BUG:` search to find more examples to mimic. + + +### Legacy error handling + +Legacy error handling macros include `Must()`, `Must3()`, and `TexcHere()`. Do +not use them in new code. The information below is provided to guide the +replacement of legacy calls with their modern equivalents. + +* Legacy `Must()` was probably meant for checking input, but developers + started to use it for checking code invariants as well. The mixture of these + two use case categories made it impossible to address various `Must()` + problems, necessitating the introduction of `Assure()` and deprecation of + `Must()` in 2022 commit b9a1bbfb. When replacing `Must()`, one has to + determine the correct use case category (as detailed in the enumerated list + at the beginning of the "Error handling" section). + +* Legacy `Must3()` is like `Must()` but supports a custom message. Existing + `Must3()` custom messages are inconsistent -- some describe the correct + condition and some describe the failure. + +* Legacy `TexcHere()` is a convenience macro. Modern code spells out + `TextException` and `Here()` explicitly. + +Keep in mind that it is usually best to leave legacy code intact. Upgrading +legacy code may be appropriate when your pull request has to modify that +specific legacy code lines for other legitimate in-scope reasons or your pull +request is actually dedicated to upgrading legacy code. In those exceptional +cases, the author becomes responsible for providing a high quality +replacement, of course. + +### Special cases + +The following functions are not covered by this documentation. They should be +avoided in most cases: `xassert()`. + ## See Also From d1baffd644c60555f394927337dbb557afcdadea Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 6 Jul 2026 17:53:45 -0400 Subject: [PATCH 04/12] Guidelines for custom assertion messages --- .../SquidCodingGuidelines.md | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/docs/DeveloperResources/SquidCodingGuidelines.md b/docs/DeveloperResources/SquidCodingGuidelines.md index 30976f0e..5df4494c 100644 --- a/docs/DeveloperResources/SquidCodingGuidelines.md +++ b/docs/DeveloperResources/SquidCodingGuidelines.md @@ -330,7 +330,8 @@ components at build time. There are several primary ways to handle various error conditions in Squid code. For any given context, only one is usually the correct choice. Using the list below, pick the _first_ one that matches your use case. See further below -for notes about optional custom assertion messages and legacy code. +for notes about bug workarounds, optional custom assertion messages, and +legacy code. 1. If the condition can be checked at compilation time, use `static_assert()`. Minor code adjustments to make compile-time assertions possible may be @@ -381,6 +382,25 @@ debugs(54, DBG_IMPORTANT, "ERROR: Squid BUG: wrong fd_note ID: " << fdNoteId); Do a `git grep ERROR:.Squid.BUG:` search to find more examples to mimic. +### Custom assertion messages + +Compiler-generated `static_assert()`, `Assure()`, and deprecated `Must()` +error messages spell out the specified condition. In special rare cases, it is +desirable to replace that generated message with a custom one. When doing so, +please preserve the message generation algorithm by describing what should be +happening (i.e. the expected condition) rather than what went wrong. For +example, + +```C++ +// XXX: The custom message describes the problem rather than the condition. +static_assert(sizeof(quotedOut) > 0, "quotedOut has zero length"); + +// OK: The custom message describes the condition. +static_assert(id > 0, "debugs() message ID must be positive"); +Assure2(headerSize >= SwapMetaPrefixSize, "UnpackPrefix() validates metadata length"); +``` + + ### Legacy error handling Legacy error handling macros include `Must()`, `Must3()`, and `TexcHere()`. Do From 7552d8a6b0daa0d8e17cc9517f7b77944d95c4a9 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sun, 12 Jul 2026 18:36:26 -0400 Subject: [PATCH 05/12] fixup: Polished "Squid bug workarounds" subsection --- docs/DeveloperResources/SquidCodingGuidelines.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/DeveloperResources/SquidCodingGuidelines.md b/docs/DeveloperResources/SquidCodingGuidelines.md index 5df4494c..1908d534 100644 --- a/docs/DeveloperResources/SquidCodingGuidelines.md +++ b/docs/DeveloperResources/SquidCodingGuidelines.md @@ -370,16 +370,16 @@ legacy code. ### Squid bug workarounds -In special rare cases, the correct choice is `Assure()`, but it is possible to -implementing a temporary workaround that would be much better than killing the -transaction. In this case, it may be permissible to emulate `Assure()` -reporting without throwing an exception: +In special rare cases, implementing a temporary bug workaround would be much +better than killing the affected transaction or Squid. In such cases, produce +`DBG_CRITICAL` or `DBG_IMPORTANT` _reporting_ with `ERROR` _and_ `Squid BUG` +but labels without calling `Assure()` or `assert()`. ```C++ -debugs(54, DBG_IMPORTANT, "ERROR: Squid BUG: wrong fd_note ID: " << fdNoteId); +debugs(33, DBG_IMPORTANT, "ERROR: Squid BUG: ConnStateData did not close " << clientConnection); ``` -Do a `git grep ERROR:.Squid.BUG:` search to find more examples to mimic. +More good examples can be found among `git grep ERROR:.Squid.BUG:` matches. ### Custom assertion messages From 5da06130b7a2f3887b63dcc9121229a8310c1edb Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sun, 12 Jul 2026 18:57:05 -0400 Subject: [PATCH 06/12] fixup: Minor polishing touches --- .../SquidCodingGuidelines.md | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/docs/DeveloperResources/SquidCodingGuidelines.md b/docs/DeveloperResources/SquidCodingGuidelines.md index 1908d534..9cc52915 100644 --- a/docs/DeveloperResources/SquidCodingGuidelines.md +++ b/docs/DeveloperResources/SquidCodingGuidelines.md @@ -327,11 +327,11 @@ components at build time. ## Error handling -There are several primary ways to handle various error conditions in Squid -code. For any given context, only one is usually the correct choice. Using the +There are several primary ways to handle error conditions in Squid code. For +any given context, only one approach is usually the correct choice. Using the list below, pick the _first_ one that matches your use case. See further below -for notes about bug workarounds, optional custom assertion messages, and -legacy code. +for notes about such special rare cases as bug workarounds, optional custom +assertion messages, and legacy code. 1. If the condition can be checked at compilation time, use `static_assert()`. Minor code adjustments to make compile-time assertions possible may be @@ -353,19 +353,19 @@ legacy code. affect just the transaction checking it, use `Assure()`. In most cases, `Assure()` failures kill the checking transaction but keep its kid process alive. Neither outcome is guaranteed though because Squid may catch and - handle the exception before it kills the transaction or the exception may + handle the exception before it kills the transaction, or the exception may propagate to the top level where it kills the kid process. 4. If the condition describes some input characteristics (e.g., "the client sent a syntactically valid HTTP request to Squid"), do not use any of the above calls. Instead, create and throw a `TextException` object, return `std::nullopt`, or otherwise signal the problem to the caller. In most - cases, adding a level-0/1 `debugs()` message is not a good idea. This is - especially true when Squid cache administrator can do nothing about that - bad input, and that bad input does not represent some very unusual or - dangerous situation. Most input validation failures ought to be reflected - in various error details logged to `access.log`, not level-0/1 `cache.log` - messages. + cases, adding a level-0/1 `debugs()` `ERROR` or `WARNING` message is _not_ + a good idea. This is especially true when Squid cache administrator can do + nothing about that bad input, and that bad input does not represent some + very unusual or dangerous situation. Most transaction-related input + validation failures ought to be reflected in various error details logged + to `access.log`, not level-0/1 `cache.log` messages. ### Squid bug workarounds @@ -384,18 +384,18 @@ More good examples can be found among `git grep ERROR:.Squid.BUG:` matches. ### Custom assertion messages -Compiler-generated `static_assert()`, `Assure()`, and deprecated `Must()` -error messages spell out the specified condition. In special rare cases, it is -desirable to replace that generated message with a custom one. When doing so, -please preserve the message generation algorithm by describing what should be -happening (i.e. the expected condition) rather than what went wrong. For -example, +Compiler-generated `static_assert()`, `Assure()`, `assert()`, and deprecated +`Must()` error messages spell out the specified condition. In special rare +cases, it is desirable to replace that generated message with a custom one. +When doing so, please preserve the message generation algorithm by describing +what should be happening (i.e. the expected condition) rather than what went +wrong. For example, ```C++ -// XXX: The custom message describes the problem rather than the condition. +// XXX: This bad custom message describes the problem rather than the condition: static_assert(sizeof(quotedOut) > 0, "quotedOut has zero length"); -// OK: The custom message describes the condition. +// OK: These custom messages describe the condition: static_assert(id > 0, "debugs() message ID must be positive"); Assure2(headerSize >= SwapMetaPrefixSize, "UnpackPrefix() validates metadata length"); ``` From 9b18729c0f58a50d4953fcc2059035d2a1a731a1 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sun, 12 Jul 2026 19:07:40 -0400 Subject: [PATCH 07/12] Applied "Squid code should throw when an invariant is violated" ... conclusion from https://github.com/measurement-factory/squid-notes/blob/start/errors.md --- .../SquidCodingGuidelines.md | 32 ++++++++----------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/docs/DeveloperResources/SquidCodingGuidelines.md b/docs/DeveloperResources/SquidCodingGuidelines.md index 9cc52915..848f9b10 100644 --- a/docs/DeveloperResources/SquidCodingGuidelines.md +++ b/docs/DeveloperResources/SquidCodingGuidelines.md @@ -340,23 +340,13 @@ assertion messages, and legacy code. assertion working is usually a bad idea. 2. If the condition describes a code invariant (e.g., "our caller must supply - a non-nil pointer"), and the bug violating that invariant is likely to - affect transactions that do not check this condition (e.g., a cache may get - corrupted, feeding other/independent transactions bogus response data), use - `assert(3)`. The guarantees provided by `assert()` are significant, but - they do not automatically extend beyond a single kid process. There is - currently no mechanism that is guaranteed to terminate the entire SMP Squid - instance. - -3. If the condition describes a code invariant (e.g., "our caller must supply - a non-nil pointer"), and the bug violating that invariant is likely to - affect just the transaction checking it, use `Assure()`. In most cases, - `Assure()` failures kill the checking transaction but keep its kid process - alive. Neither outcome is guaranteed though because Squid may catch and - handle the exception before it kills the transaction, or the exception may - propagate to the top level where it kills the kid process. - -4. If the condition describes some input characteristics (e.g., "the client + a non-nil pointer"), use `Assure()`. In most cases, `Assure()` failures + kill the checking transaction but keep its kid process alive. Neither + outcome is guaranteed though because Squid may catch and handle the + exception before it kills the transaction, or the exception may propagate + to the top level where it kills the kid process. + +3. If the condition describes some input characteristics (e.g., "the client sent a syntactically valid HTTP request to Squid"), do not use any of the above calls. Instead, create and throw a `TextException` object, return `std::nullopt`, or otherwise signal the problem to the caller. In most @@ -373,7 +363,7 @@ assertion messages, and legacy code. In special rare cases, implementing a temporary bug workaround would be much better than killing the affected transaction or Squid. In such cases, produce `DBG_CRITICAL` or `DBG_IMPORTANT` _reporting_ with `ERROR` _and_ `Squid BUG` -but labels without calling `Assure()` or `assert()`. +but labels without calling `Assure()`. ```C++ debugs(33, DBG_IMPORTANT, "ERROR: Squid BUG: ConnStateData did not close " << clientConnection); @@ -429,7 +419,11 @@ request is actually dedicated to upgrading legacy code. In those exceptional cases, the author becomes responsible for providing a high quality replacement, of course. -### Special cases +### Other special cases + +In code residing outside of `src/`, in helper code that has not been upgraded +to use `src/base` APIs, and in legacy C code, `Assure()` is not available. Use +`assert()`. The following functions are not covered by this documentation. They should be avoided in most cases: `xassert()`. From 7d4a6ea758eed12b4f47109c51d6a9200eeac21f Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sun, 12 Jul 2026 20:14:34 -0400 Subject: [PATCH 08/12] Avoid fatal() and friends ... per https://github.com/measurement-factory/squid-notes/blob/start/errors.md#problems-with-fatal --- docs/DeveloperResources/SquidCodingGuidelines.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/DeveloperResources/SquidCodingGuidelines.md b/docs/DeveloperResources/SquidCodingGuidelines.md index 848f9b10..8e7bb130 100644 --- a/docs/DeveloperResources/SquidCodingGuidelines.md +++ b/docs/DeveloperResources/SquidCodingGuidelines.md @@ -426,7 +426,8 @@ to use `src/base` APIs, and in legacy C code, `Assure()` is not available. Use `assert()`. The following functions are not covered by this documentation. They should be -avoided in most cases: `xassert()`. +avoided in most cases, especially in new code: `fatalf()`, `fatal()`, +`fatal_dump()`, xassert()`. ## See Also From 1490d06d166ffa40b2518aba8295cf7f731fdf87 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sun, 12 Jul 2026 20:17:27 -0400 Subject: [PATCH 09/12] fixup: Minor polishing --- docs/DeveloperResources/SquidCodingGuidelines.md | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/docs/DeveloperResources/SquidCodingGuidelines.md b/docs/DeveloperResources/SquidCodingGuidelines.md index 8e7bb130..acd5a60b 100644 --- a/docs/DeveloperResources/SquidCodingGuidelines.md +++ b/docs/DeveloperResources/SquidCodingGuidelines.md @@ -363,7 +363,7 @@ assertion messages, and legacy code. In special rare cases, implementing a temporary bug workaround would be much better than killing the affected transaction or Squid. In such cases, produce `DBG_CRITICAL` or `DBG_IMPORTANT` _reporting_ with `ERROR` _and_ `Squid BUG` -but labels without calling `Assure()`. +labels but without calling `Assure()`. ```C++ debugs(33, DBG_IMPORTANT, "ERROR: Squid BUG: ConnStateData did not close " << clientConnection); @@ -419,15 +419,16 @@ request is actually dedicated to upgrading legacy code. In those exceptional cases, the author becomes responsible for providing a high quality replacement, of course. + ### Other special cases -In code residing outside of `src/`, in helper code that has not been upgraded -to use `src/base` APIs, and in legacy C code, `Assure()` is not available. Use -`assert()`. +`Assure()` is not available in code residing outside of `src/`, in helper code +that has not been upgraded to use `src/base` APIs, and in legacy C code. Use +`assert()` instead. -The following functions are not covered by this documentation. They should be -avoided in most cases, especially in new code: `fatalf()`, `fatal()`, -`fatal_dump()`, xassert()`. +The following error handling functions are not covered by this documentation. +They should be avoided in most cases, especially in new code: `fatalf()`, +`fatal()`, `fatal_dump()`, xassert()`. ## See Also From 47a095c3c2b503d68f3e8e95fbee939d626eb187 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sun, 12 Jul 2026 20:26:18 -0400 Subject: [PATCH 10/12] Cover "unreachable code" cases --- .../SquidCodingGuidelines.md | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/docs/DeveloperResources/SquidCodingGuidelines.md b/docs/DeveloperResources/SquidCodingGuidelines.md index acd5a60b..69ca501c 100644 --- a/docs/DeveloperResources/SquidCodingGuidelines.md +++ b/docs/DeveloperResources/SquidCodingGuidelines.md @@ -330,8 +330,8 @@ components at build time. There are several primary ways to handle error conditions in Squid code. For any given context, only one approach is usually the correct choice. Using the list below, pick the _first_ one that matches your use case. See further below -for notes about such special rare cases as bug workarounds, optional custom -assertion messages, and legacy code. +for notes about such special rare cases as bug workarounds, unreachable code, +optional custom assertion messages, and legacy code. 1. If the condition can be checked at compilation time, use `static_assert()`. Minor code adjustments to make compile-time assertions possible may be @@ -372,6 +372,21 @@ debugs(33, DBG_IMPORTANT, "ERROR: Squid BUG: ConnStateData did not close " << cl More good examples can be found among `git grep ERROR:.Squid.BUG:` matches. +### Unreachable code + +Unreachable code is special because there is no meaningful condition to be +evaluated inside that code (unless you consider `true` to be meaningful). +Reaching an unreachable code is a Squid bug. If this bug can be detected at +compile time, use `#error` preprocessor instruction. Otherwise, use `Assure()` +with the following always-false condition pattern: + +```C++ +Assure(!"invariant description"); +``` + +Good examples can be found among `git grep 'Assure.!"'` matches. + + ### Custom assertion messages Compiler-generated `static_assert()`, `Assure()`, `assert()`, and deprecated From 235f7b2c23d64ce64cc4eed4b2d241fd758dd5db Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 14 Jul 2026 11:43:36 -0400 Subject: [PATCH 11/12] Moved details to docs/DeveloperResources/ErrorHandling Co-authored-by: Francesco Chemolli --- docs/DeveloperResources/ErrorHandling.md | 129 +++++++++++++++++ .../SquidCodingGuidelines.md | 135 ++---------------- 2 files changed, 142 insertions(+), 122 deletions(-) create mode 100644 docs/DeveloperResources/ErrorHandling.md diff --git a/docs/DeveloperResources/ErrorHandling.md b/docs/DeveloperResources/ErrorHandling.md new file mode 100644 index 00000000..46461296 --- /dev/null +++ b/docs/DeveloperResources/ErrorHandling.md @@ -0,0 +1,129 @@ +--- +--- +# Error Handling + +This page details +[SquidCodingGuidelines](/DeveloperResources/SquidCodingGuidelines) related to +validating code invariants and input. + + +## Primary API choices + +There are several primary ways to handle error conditions in Squid code. For +any given context, only one approach is usually the correct choice. Using the +list below, pick the _first_ one that matches your use case. See further below +for notes about such special rare cases as bug workarounds, unreachable code, +optional custom assertion messages, and legacy code. + +1. If the condition can be checked at compilation time, use `static_assert()`. + Minor code adjustments to make compile-time assertions possible may be + allowed, but Squid currently avoids explicit `constexpr`, and sprinkling + Squid code with many `constexpr` specifiers to get some compile-time + assertion working is usually a bad idea. + +2. If the condition describes a code invariant (e.g., "our caller must supply + a non-nil pointer"), use `Assure()`. In most cases, `Assure()` failures + kill the checking transaction but keep its kid process alive. Neither + outcome is guaranteed though because Squid may catch and handle the + exception before it kills the transaction, or the exception may propagate + to the top level where it kills the kid process. + +3. If the condition describes some input characteristics (e.g., "the client + sent a syntactically valid HTTP request to Squid"), do not use any of the + above calls. Instead, create and throw a `TextException` object, return + `std::nullopt`, or otherwise signal the problem to the caller. In most + cases, adding a level-0/1 `debugs()` `ERROR` or `WARNING` message is _not_ + a good idea. This is especially true when Squid cache administrator can do + nothing about that bad input, and that bad input does not represent some + very unusual or dangerous situation. Most transaction-related input + validation failures ought to be reflected in various error details logged + to `access.log`, not level-0/1 `cache.log` messages. + + +## Squid bug workarounds + +In special rare cases, implementing a temporary bug workaround would be much +better than killing the affected transaction or Squid. In such cases, produce +`DBG_CRITICAL` or `DBG_IMPORTANT` _reporting_ with `ERROR` _and_ `Squid BUG` +labels but without calling `Assure()`. + +```C++ +debugs(33, DBG_IMPORTANT, "ERROR: Squid BUG: ConnStateData did not close " << clientConnection); +``` + +More good examples can be found among `git grep ERROR:.Squid.BUG:` matches. + + +## Unreachable code + +Unreachable code is special because there is no meaningful condition to be +evaluated inside that code (unless you consider `true` to be meaningful). +Reaching an unreachable code is a Squid bug. If this bug can be detected at +compile time, use `#error` preprocessor instruction. Otherwise, use `Assure()` +with the following always-false condition pattern: + +```C++ +Assure(!"invariant description"); +``` + +Good examples can be found among `git grep 'Assure.!"'` matches. + + +## Custom assertion messages + +Compiler-generated `static_assert()`, `Assure()`, `assert()`, and deprecated +`Must()` error messages spell out the specified condition. In special rare +cases, it is desirable to replace that generated message with a custom one. +When doing so, please preserve the message generation algorithm by describing +what should be happening (i.e. the expected condition) rather than what went +wrong. For example, + +```C++ +// XXX: This bad custom message describes the problem rather than the condition: +static_assert(sizeof(quotedOut) > 0, "quotedOut has zero length"); + +// OK: These custom messages describe the condition: +static_assert(id > 0, "debugs() message ID must be positive"); +Assure2(headerSize >= SwapMetaPrefixSize, "UnpackPrefix() validates metadata length"); +``` + + +## Legacy error handling + +Legacy error handling macros include `Must()`, `Must3()`, and `TexcHere()`. Do +not use them in new code. The information below is provided to guide the +replacement of legacy calls with their modern equivalents. + +* Legacy `Must()` was probably meant for checking input, but developers + started to use it for checking code invariants as well. The mixture of these + two use case categories made it impossible to address various `Must()` + problems, necessitating the introduction of `Assure()` and deprecation of + `Must()` in 2022 commit b9a1bbfb. When replacing `Must()`, one has to + determine the correct use case category (as detailed in the enumerated list + at the beginning of the "Error handling" section). + +* Legacy `Must3()` is like `Must()` but supports a custom message. Existing + `Must3()` custom messages are inconsistent -- some describe the correct + condition and some describe the failure. + +* Legacy `TexcHere()` is a convenience macro. Modern code spells out + `TextException` and `Here()` explicitly. + +Keep in mind that it is usually best to leave legacy code intact. Upgrading +legacy code may be appropriate when your pull request has to modify that +specific legacy code lines for other legitimate in-scope reasons or your pull +request is actually dedicated to upgrading legacy code. In those exceptional +cases, the author becomes responsible for providing a high quality +replacement, of course. + + +## Other special cases + +`Assure()` is not available in code residing outside of `src/`, in helper code +that has not been upgraded to use `src/base` APIs, and in legacy C code. Use +`assert()` instead. + +The following error handling functions are not covered by this documentation. +They should be avoided in most cases, especially in new code: `fatalf()`, +`fatal()`, `fatal_dump()`, xassert()`. + diff --git a/docs/DeveloperResources/SquidCodingGuidelines.md b/docs/DeveloperResources/SquidCodingGuidelines.md index 69ca501c..ec359bd9 100644 --- a/docs/DeveloperResources/SquidCodingGuidelines.md +++ b/docs/DeveloperResources/SquidCodingGuidelines.md @@ -57,6 +57,19 @@ formater. * Naming conventions as covered in [Features/SourceLayout](/Features/SourceLayout) are to be used. +* To assert a condition that can be checked at compilation time, use + `static_assert()`. See [ErrorHandling](/DeveloperResources/ErrorHandling) + for details. +* To confirm a code invariant that cannot be checked at compilation time, use + `Assure()` if you can and `assert()` otherwise. See + [ErrorHandling](/DeveloperResources/ErrorHandling) for details. +* Do not use `Assure()` or `assert()` to validate input. Instead, create and + throw a `TextException` object, return `std::nullopt`, or otherwise signal + the problem to the caller. See + [ErrorHandling](/DeveloperResources/ErrorHandling) for details. +* Do not use deprecated `Must()`, `Must3()`, fatalf()`, `fatal()`, and + `fatal_dump()` in new code. See + [ErrorHandling](/DeveloperResources/ErrorHandling) for details. ### Rule: No new globals @@ -324,128 +337,6 @@ components at build time. * MUST be used inside .h to wrap relevant code. - -## Error handling - -There are several primary ways to handle error conditions in Squid code. For -any given context, only one approach is usually the correct choice. Using the -list below, pick the _first_ one that matches your use case. See further below -for notes about such special rare cases as bug workarounds, unreachable code, -optional custom assertion messages, and legacy code. - -1. If the condition can be checked at compilation time, use `static_assert()`. - Minor code adjustments to make compile-time assertions possible may be - allowed, but Squid currently avoids explicit `constexpr`, and sprinkling - Squid code with many `constexpr` specifiers to get some compile-time - assertion working is usually a bad idea. - -2. If the condition describes a code invariant (e.g., "our caller must supply - a non-nil pointer"), use `Assure()`. In most cases, `Assure()` failures - kill the checking transaction but keep its kid process alive. Neither - outcome is guaranteed though because Squid may catch and handle the - exception before it kills the transaction, or the exception may propagate - to the top level where it kills the kid process. - -3. If the condition describes some input characteristics (e.g., "the client - sent a syntactically valid HTTP request to Squid"), do not use any of the - above calls. Instead, create and throw a `TextException` object, return - `std::nullopt`, or otherwise signal the problem to the caller. In most - cases, adding a level-0/1 `debugs()` `ERROR` or `WARNING` message is _not_ - a good idea. This is especially true when Squid cache administrator can do - nothing about that bad input, and that bad input does not represent some - very unusual or dangerous situation. Most transaction-related input - validation failures ought to be reflected in various error details logged - to `access.log`, not level-0/1 `cache.log` messages. - - -### Squid bug workarounds - -In special rare cases, implementing a temporary bug workaround would be much -better than killing the affected transaction or Squid. In such cases, produce -`DBG_CRITICAL` or `DBG_IMPORTANT` _reporting_ with `ERROR` _and_ `Squid BUG` -labels but without calling `Assure()`. - -```C++ -debugs(33, DBG_IMPORTANT, "ERROR: Squid BUG: ConnStateData did not close " << clientConnection); -``` - -More good examples can be found among `git grep ERROR:.Squid.BUG:` matches. - - -### Unreachable code - -Unreachable code is special because there is no meaningful condition to be -evaluated inside that code (unless you consider `true` to be meaningful). -Reaching an unreachable code is a Squid bug. If this bug can be detected at -compile time, use `#error` preprocessor instruction. Otherwise, use `Assure()` -with the following always-false condition pattern: - -```C++ -Assure(!"invariant description"); -``` - -Good examples can be found among `git grep 'Assure.!"'` matches. - - -### Custom assertion messages - -Compiler-generated `static_assert()`, `Assure()`, `assert()`, and deprecated -`Must()` error messages spell out the specified condition. In special rare -cases, it is desirable to replace that generated message with a custom one. -When doing so, please preserve the message generation algorithm by describing -what should be happening (i.e. the expected condition) rather than what went -wrong. For example, - -```C++ -// XXX: This bad custom message describes the problem rather than the condition: -static_assert(sizeof(quotedOut) > 0, "quotedOut has zero length"); - -// OK: These custom messages describe the condition: -static_assert(id > 0, "debugs() message ID must be positive"); -Assure2(headerSize >= SwapMetaPrefixSize, "UnpackPrefix() validates metadata length"); -``` - - -### Legacy error handling - -Legacy error handling macros include `Must()`, `Must3()`, and `TexcHere()`. Do -not use them in new code. The information below is provided to guide the -replacement of legacy calls with their modern equivalents. - -* Legacy `Must()` was probably meant for checking input, but developers - started to use it for checking code invariants as well. The mixture of these - two use case categories made it impossible to address various `Must()` - problems, necessitating the introduction of `Assure()` and deprecation of - `Must()` in 2022 commit b9a1bbfb. When replacing `Must()`, one has to - determine the correct use case category (as detailed in the enumerated list - at the beginning of the "Error handling" section). - -* Legacy `Must3()` is like `Must()` but supports a custom message. Existing - `Must3()` custom messages are inconsistent -- some describe the correct - condition and some describe the failure. - -* Legacy `TexcHere()` is a convenience macro. Modern code spells out - `TextException` and `Here()` explicitly. - -Keep in mind that it is usually best to leave legacy code intact. Upgrading -legacy code may be appropriate when your pull request has to modify that -specific legacy code lines for other legitimate in-scope reasons or your pull -request is actually dedicated to upgrading legacy code. In those exceptional -cases, the author becomes responsible for providing a high quality -replacement, of course. - - -### Other special cases - -`Assure()` is not available in code residing outside of `src/`, in helper code -that has not been upgraded to use `src/base` APIs, and in legacy C code. Use -`assert()` instead. - -The following error handling functions are not covered by this documentation. -They should be avoided in most cases, especially in new code: `fatalf()`, -`fatal()`, `fatal_dump()`, xassert()`. - - ## See Also * [DeveloperResources](/DeveloperResources). From eee64ef129bf525925700423c5ace34466ea704f Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 14 Jul 2026 11:48:29 -0400 Subject: [PATCH 12/12] fixup: Markdown typo --- docs/DeveloperResources/ErrorHandling.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/DeveloperResources/ErrorHandling.md b/docs/DeveloperResources/ErrorHandling.md index 46461296..0e073be6 100644 --- a/docs/DeveloperResources/ErrorHandling.md +++ b/docs/DeveloperResources/ErrorHandling.md @@ -125,5 +125,5 @@ that has not been upgraded to use `src/base` APIs, and in legacy C code. Use The following error handling functions are not covered by this documentation. They should be avoided in most cases, especially in new code: `fatalf()`, -`fatal()`, `fatal_dump()`, xassert()`. +`fatal()`, `fatal_dump()`, `xassert()`.