[squid-dev] [PATCH] VIA creation code duplication

[Alex Rousskov]

On 03/22/2017 07:20 AM, Amos Jeffries wrote:
> On 17/03/2017 6:17 a.m., Alex Rousskov wrote:
>> On 03/16/2017 05:15 AM, Amos Jeffries wrote:
>>
>>
>>> Any objections to applying this with this added:
>>>
>>>   // XXX: putStr() still has String 64KB limits
>>>   Must(strVia.length() < 64*1024);
>>
>> No objections from me if you replace the magic constant with a new
>> inlined String::MaxSizeXXX() method. The slightly misleading source code
>> comment above becomes unnecessary after that. Why XXX()? Because no
>> correct caller code should know about internal String limits (but it may
>> catch exceptions, including exceptions due to those limits).


> it seems reasonable for a caller to expect there to be _a_ limit.

Yes, although it is easy to misinterpret "expect" in this context. Any
concatenation or append operation may fail because it hits some kind of
a limit. We should go even further: Almost any operation may fail. This
is a fact. This fact should be common knowledge, and the code should
"expect" such failures. However, "expect" here usually does not imply
"prevent". It usually means "handle". The critical difference becomes
important in the second part of my answer to your question(**).

Moreover, since almost everything can fail, good programs written in
modern languages do not handle failures the same way programs written in
C do. The old (and failed)

C: "check each return value for being invalid and immediately treat an
invalid value as an error"

approach becomes

C++: "only return valid values but anticipate exceptions and catch them
where it is convenient"

or

Rust: "do not return raw values and unwrap either-value-or-error bundles
(and handle errors) where it is necessary to access the raw value" (AFAIK)


> It is also reasonable for the caller to be aware of some API way
> to find it out.

It is certainly tempting but usually wrong. The exact limit, which
operations it applies to, and how exactly it is applied is an internal
implementation detail. In the case of a SomeString class, for example,
the fact that some String classes always append the null terminator,
some never do, and some append under certain conditions may affect how
the limit is applied. Burdening the caller with all that internal String
knowledge is clearly wrong.

As mentioned earlier(**), the correct approach is to handle the
situation where the caller has hit some limit and the called operation
has thrown an exception. Since we do not want to (and often cannot
reliably) prevent our actions from hitting limits, exposing some limit
is unnecessary, and the code using such an exposed limit is likely to be
or become buggy. This is why we should add and XXX suffix to the
limit-returning method that encourages writing such code.

Various buggy versions of the patch in question (and earlier Squid code
suffering from similar crashes) illustrate the advantages of the "do not
try to prevent errors because you will fail; throw and handle exceptions
instead" approach well.

There are certainly exceptional situations where these general
approaches either do not apply at all or cannot be applied until the
code is restructured/fixed. In the latter cases, we add XXXs in attempt
to mark bad code (even if that code is currently necessary) and slow
down the propagation of that bad code or its bad idea.


>>> (I kind of hate those low-level operations with Must() and assert() like
>>> the former - far too little context to be useful debugging with such big
>>> impact when they are triggered.)
>>
>> While it is indeed tempting to hate those Must(), we must keep in mind
>> that low-level assert-replacing Must()s are not meant for debugging or
>> even traditional error handling. Their purpose is to (hopefully) prevent
>> Squid death.
>>
>> When the crash avoidance project completes, we will have a way to dump
>> stack traces for specific Must()s, which will provide the same level of
>> triage info as an assert() would, but (hopefully) without killing Squid
>> in environments where such functionality is supported.
>>
> 
> Any kind of estimate on that?

2017 or so. No sponsor gives this work high priority so it is currently
a barely moving background project.


Cheers,

Alex.