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

On Thu, 2008-04-17 at 19:28 +0300, Tsantilas Christos wrote:
> Alex Rousskov wrote:
> >
> >> 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?
> >
>
> I will try both, to see what is better.

> The ICAPModXact::parseBody maybe is a better place because we can
> informed about the parse error and handle it as a "parse error" and
> maybe disable the buggy ICAP service.

And/or bypass the parsing error. I think the existing ICAP transaction
should already do the above. No changes are necessary at a high-level.
We are being bitten by the assertion triggered by an exception. Once
that assertion is avoided or converted to something more manageable, we
should be fine.

> If we handle it where the undoCheckOut called we do not know where
> exactly is the problem.

I meant throwing in undoCheckOut, not handling the exception there.
Unfortunately, throwing was a bad idea because if we throw a new
exception while another exception is being processed, the "terminate()"
function is called (because it would be awkward to catch two or more
exceptions at the same time).

What's wrong here is that our checkout object destructor calls things
that may throw (or assert) but does not handle that. It is a well-known
rule that a destructor must not throw.

You are right that we cannot handle the exception/assertion in BodyPipe
or related classes because those classes do not have the functionality
to recover from a failed checkout yet.

> The related asserts are replaced with Must commands (only in async-call
> branch), and I think BodyPipe in squid3-trunk can handle better the
> cases where a problem occurred in the one part of the pipe.
>
> For example in squid3.0 simply catching the exception is not enough
> because the client-side part of the pipe does not informed about the
> error and try to continue using the pipe and squid hits other assertions.

Understood.

I suggest the following:

Short-term: surround the parsing call with try/catch. Handle the parsing
exception by committing the buffer as if there was no error and aborting
the transaction. Do it in v3.0 and v3.1. This should be OK because the
existing parser should not leave the buffer in an inconsistent state.

Long-term: Remove the above hack. Add code to BodyPipe to mark the
internal buffer as "invalid". Any external access to such buffer would
result in an exception being thrown. If the checkout object destructor
catches an exception, it would invalidate the BodyPipe buffer rather
than letting the exception escape. This would make the checkout process
exception-safe even in the presence of an externally thrown exception.

Thank you,

Alex.
Received on Tue Apr 22 2008 - 15:17:09 MDT