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

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? It
will work in trunk, right?

> The problem does not exists in squid3 async-calls branch.

Do you understand why? Can you explain?

Thank you,

Alex.
Received on Tue Apr 22 2008 - 16:16:49 MDT