Re: [Bug 2311] SQUID crashes/ restarts when ICAP enabled on respmod for HTTP body size greater than 100kb

Alex Rousskov wrote:
> On Wed, 2008-04-16 at 14:00 -0600, bugzilla-daemon@squid-cache.org
> wrote:
>> http://www.squid-cache.org/bugs/show_bug.cgi?id=2311
>
>> --- Comment #5 from Christos Tsantilas <chtsanti@users.sourceforge.net> 2008-04-16 14:00:28 ---
>> Here is the problem:
>> 1) We are in the ICAPModXact::parseBody method the "bpc" a BodyPipeCheckout
>> object created.
>> 2) The ChunkedCodingParser::parse method called (line:
>> bodyParser->parse(&readBuf, &bpc.buf) )
>> 3) Inside the ChunkedCodingParser::parse a parse error happens so a TexcHere
>> exception thrown.
>> - The ICAPModXact::parseBody aborted immediately and the "bpc" object
>> destroyed so the BodyPipeCheckout::~BodyPipeCheckout destructor called.
>> 4) The BodyPipeCheckout::checkedIn now is false so the pipe.undoCheckOut
>> called.
>> 5) Now inside the BodyPipe::undoCheckOut method the checkout.checkedOutSize
>> for all of my test cases is zero and the currentSize are the successfully
>> parsed data, so we are hitting the assertion "checkout.checkedOutSize ==
>> currentSize".
>
> Christos,
>
> Thanks a lot for discovering the cause of this problem.
>
>> I can not understand where the BodyPipe::undoCheckOut is useful, and for
>> sure does not work.
>
> Actually, it sounds like BodyPipe::undoCheckOut does exactly what it was
> supposed to do -- it asserts because the body pipe code cannot undo from
> the parsing error of that kind. We should probably change that assertion
> to a Must() so that it cancels the ICAP transaction rather than killing
> Squid (more on that below).
>
> Please let me explain the rationale behind the checkout API. BodyPipe
> maintains a pipe or a buffer. The class provides several guarantees. For
> example, the consumer is notified when new data is added. Those
> guarantees are very important. The integrity of the pipe must be
> preserved to provide them. This is done by using public BodyPipe
> methods, as usual.
>
> In some cases, the user cannot or does not want to manipulate BodyPipe
> directly (in our case the parser is not BodyPipe aware). In those cases,
> the user can "checkout" the raw buffer for a "second" and modify that
> buffer directly. When the user returns the buffer, the pipe notices the
> changes and applies them as a single/atomic transaction, with all the
> guarantees preserved.
>
> The code is written so that if something goes wrong when the raw buffer
> is checked out, the checkout is aborted. The pipe tries to recover from
> that abort, but if the raw buffer was modified, it currently cannot do
> that. One of the reasons behind this undo failure is that it is not
> clear that the pending changes in the raw buffer are
> complete/consistent. The code can be made smarter, but there always be
> corner cases and most smarts will affect code overheads (which are
> currently negligible).
>
> In our case, the parser adds a little bit of parsed content and then
> discovers a corrupted chunk. The parser throws, as it should. The
> BodyPipe cannot undo that change, and that's OK. What it should not do
> is assert. It should throw instead. I think this was not done originally
> because I did not want to add TextExceptions to files outside of ICAP/.
> That is no longer a problem (at least not in trunk).
>
>> I think, the only solution for squid3.0 is to try to handle the exceptions
>> thrown by ChunkedCodingParser::parse method, inside the ICAPModXact::parseBody
>> method ...
>
> What about throwing from undoCheckOut? Would that work in Squid 3.0?

I don't think it will work in 3.0. Unless you want to 'port' a
try...catch for it specifically, just to keep squid running and kill the
individual connection.

> It
> will work in trunk, right?

I assume so.

>
>> The problem does not exists in squid3 async-calls branch.
>
> Do you understand why? Can you explain?
>
> Thank you,
>
> Alex.
>

Amos

-- 
Please use Squid 2.6.STABLE19 or 3.0.STABLE4