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. _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev