On Wed, 2008-04-16 at 14:00 -0600, [EMAIL PROTECTED] wrote: > http://www.squid-cache.org/bugs/show_bug.cgi?id=2311
> --- Comment #5 from Christos Tsantilas <[EMAIL PROTECTED]> 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.