--------------------- PatchSet 4058 Date: 2007/02/20 16:22:30 Author: rousskov Branch: squid3-icap Tag: (none) Log: - Fixed ieof condition detection. Squid was sending last-chunk without ieof bit and was sending two last chunks when doing preview (Tsantilas Christos). - When ICAP server wants the entire virgin body and sends 100 Continue after Preview, do not stop backing up virgin body data for echoing if we promised to support 204 No Content responses outside of Preview. If we allow 204s, 100 Continue may be followed by a 204 No Content and we will need the entire virgin body to echo back. - Rewrote MemBufClaim into a VirginBodyAct class to simplify and clarify code in hope to minimize the number of bugs like the one mentioned above. MemBufClaim was protecting an area of virgin body pipe content and, as a side effect, was providing the transaction with the content pointer for the write or send actions. Now VirginBodyAct just maintains the activity offset and the transaction code uses that to consume virgin body correctly. The size of the area is no longer maintained because it was usually unknown or unused; and when it was known and used (i.e., Preview), it could be taken from the preview state object anyway. Renamed and documented VirginBodyAct-related methods to clarify the intent. Members: src/ICAP/ICAPModXact.cc:1.1.2.21->1.1.2.22 src/ICAP/ICAPModXact.h:1.1.2.9->1.1.2.10 Index: squid3/src/ICAP/ICAPModXact.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPModXact.cc,v retrieving revision 1.1.2.21 retrieving revision 1.1.2.22 diff -u -r1.1.2.21 -r1.1.2.22 --- squid3/src/ICAP/ICAPModXact.cc 14 Feb 2007 22:52:30 -0000 1.1.2.21 +++ squid3/src/ICAP/ICAPModXact.cc 20 Feb 2007 16:22:30 -0000 1.1.2.22 @@ -236,12 +236,12 @@ void ICAPModXact::writePrimeBody() { Must(state.writing == State::writingPrime); - Must(virginWriteClaim.active()); + Must(virginBodyWriting.active()); const size_t size = (size_t)virgin.body_pipe->buf().contentSize(); writeSomeBody("prime virgin body", size); - if (doneWithClaim(virginWriteClaim)) { + if (virginBodyEndReached(virginBodyWriting)) { debugs(93, 5, HERE << "wrote entire body"); stopWriting(true); } @@ -258,21 +258,29 @@ writeBuf.init(); // note: we assume that last-chunk will fit - const size_t writableSize = claimSize(virginWriteClaim); + const size_t writableSize = virginContentSize(virginBodyWriting); const size_t chunkSize = XMIN(writableSize, size); if (chunkSize) { debugs(93, 7, HERE << "will write " << chunkSize << "-byte chunk of " << label); + + openChunk(writeBuf, chunkSize, false); + writeBuf.append(virginContentData(virginBodyWriting), chunkSize); + closeChunk(writeBuf); + + virginBodyWriting.progress(chunkSize); + virginConsume(); } else { debugs(93, 7, "ICAPModXact has no writable " << label << " content"); } - moveRequestChunk(writeBuf, chunkSize); // even if chunkSize == 0 - - const bool lastChunk = - (state.writing == State::writingPreview && preview.done()) || - doneWithClaim(virginWriteClaim); + const bool wroteEof = virginBodyEndReached(virginBodyWriting); + bool lastChunk = wroteEof; + if (state.writing == State::writingPreview) { + preview.wrote(chunkSize, wroteEof); // even if wrote nothing + lastChunk = lastChunk || preview.done(); + } if (lastChunk) { debugs(93, 8, HERE << "will write last-chunk of " << label); @@ -289,26 +297,6 @@ } } -void ICAPModXact::moveRequestChunk(MemBuf &buf, size_t chunkSize) -{ - if (chunkSize > 0) { - openChunk(buf, chunkSize, false); - buf.append(claimContent(virginWriteClaim), chunkSize); - closeChunk(buf); - - virginWriteClaim.release(chunkSize); - virginConsume(); - } - - if (state.writing == State::writingPreview) { - // if there is no active virginWriteClaim, - // then there is no body left to write, - // so we must have written everything - const bool wroteEof = !virginWriteClaim.active(); - preview.wrote(chunkSize, wroteEof); // even if wrote nothing - } -} - void ICAPModXact::addLastRequestChunk(MemBuf &buf) { const bool ieof = state.writing == State::writingPreview && preview.ieof(); @@ -326,29 +314,32 @@ buf.append(ICAP::crlf, 2); // chunk-terminating CRLF } -// can we stop the activity (i.e., writing or sending) protected by the claim? -bool ICAPModXact::doneWithClaim(const MemBufClaim &claim) const +// did the activity reached the end of the virgin body? +bool ICAPModXact::virginBodyEndReached(const VirginBodyAct &act) const { - // handled all (assuming the claim was originally enabled) OR - // handled everything we will ever had - return - !claim.active() || - (virgin.body_pipe->productionEnded() && claimSize(claim) <= 0); -} - -size_t ICAPModXact::claimSize(const MemBufClaim &claim) const -{ - Must(claim.active()); - const size_t start = claim.offset(); + return + !act.active() || // did all (assuming it was originally planned) + !virgin.body_pipe->expectMoreAfter(act.offset()); // wont have more +} + +// the size of buffered virgin body data available for the specified activity +// if this size is zero, we may be done or may be waiting for more data +size_t ICAPModXact::virginContentSize(const VirginBodyAct &act) const +{ + Must(act.active()); + // asbolute start of unprocessed data + const size_t start = act.offset(); + // absolute end of buffered data const size_t end = virginConsumed + virgin.body_pipe->buf().contentSize(); Must(virginConsumed <= start && start <= end); return end - start; } -const char *ICAPModXact::claimContent(const MemBufClaim &claim) const +// pointer to buffered virgin body data available for the specified activity +const char *ICAPModXact::virginContentData(const VirginBodyAct &act) const { - Must(claim.active()); - const size_t start = claim.offset(); + Must(act.active()); + const size_t start = act.offset(); Must(virginConsumed <= start); return virgin.body_pipe->buf().content() + (start-virginConsumed); } @@ -363,11 +354,11 @@ const size_t end = virginConsumed + have; size_t offset = end; - if (virginWriteClaim.active()) - offset = XMIN(virginWriteClaim.offset(), offset); + if (virginBodyWriting.active()) + offset = XMIN(virginBodyWriting.offset(), offset); - if (virginSendClaim.active()) - offset = XMIN(virginSendClaim.offset(), offset); + if (virginBodySending.active()) + offset = XMIN(virginBodySending.offset(), offset); Must(virginConsumed <= offset && offset <= end); @@ -412,19 +403,19 @@ debugs(93, 7, HERE << "will no longer write" << status()); state.writing = State::writingReallyDone; - if (virginWriteClaim.active()) { - virginWriteClaim.disable(); + if (virginBodyWriting.active()) { + virginBodyWriting.disable(); virginConsume(); } } void ICAPModXact::stopBackup() { - if (!virginSendClaim.active()) + if (!virginBodySending.active()) return; debugs(93, 7, "ICAPModXact will no longer backup" << status()); - virginSendClaim.disable(); + virginBodySending.disable(); virginConsume(); } @@ -478,21 +469,21 @@ { Must(state.sending == State::sendingVirgin); Must(adapted.body_pipe != NULL); - Must(virginSendClaim.active()); + Must(virginBodySending.active()); - const size_t sizeMax = claimSize(virginSendClaim); + const size_t sizeMax = virginContentSize(virginBodySending); debugs(93,5, HERE << "will echo up to " << sizeMax << " bytes from " << virgin.body_pipe->status() << " to " << adapted.body_pipe->status()); if (sizeMax > 0) { - const size_t size = adapted.body_pipe->putMoreData(claimContent(virginSendClaim), sizeMax); + const size_t size = adapted.body_pipe->putMoreData(virginContentData(virginBodySending), sizeMax); debugs(93,5, HERE << "echoed " << size << " out of " << sizeMax << " bytes"); - virginSendClaim.release(size); + virginBodySending.progress(size); virginConsume(); } - if (doneWithClaim(virginSendClaim)) { + if (virginBodyEndReached(virginBodySending)) { debugs(93, 5, "ICAPModXact echoed all" << status()); stopSending(true); } else { @@ -516,7 +507,7 @@ if (state.sending != State::sendingUndecided) { debugs(93, 7, "ICAPModXact will no longer send" << status()); if (adapted.body_pipe != NULL) { - virginSendClaim.disable(); + virginBodySending.disable(); // we may leave debts if we were echoing and the virgin // body_pipe got exhausted before we echoed all planned bytes const bool leftDebts = adapted.body_pipe->needsMoreData(); @@ -666,7 +657,8 @@ // server must not respond before the end of preview: we may send ieof Must(preview.enabled() && preview.done() && !preview.ieof()); - if (virginSendClaim.active() && virginSendClaim.limited()) // preview only + // 100 "Continue" cancels our preview commitment, not 204s outside preview + if (!state.allowedPostview204) stopBackup(); state.parsing = State::psIcapHeader; // eventually @@ -735,8 +727,7 @@ oldHead->body_pipe); state.sending = State::sendingVirgin; checkConsuming(); - Must(virginSendClaim.active()); - virginSendClaim.protectAll(); // extends protection if needed + Must(virginBodySending.active()); // TODO: optimize: is it possible to just use the oldHead pipe and // remove ICAP from the loop? This echoing is probably a common case! makeAdaptedBodyPipe("echoed virgin response"); @@ -1015,16 +1006,17 @@ if (shouldPreview(urlPath)) { buf.Printf("Preview: %d\r\n", (int)preview.ad()); if (virginBody.expected()) // there is a body to preview - virginSendClaim.protectUpTo(preview.ad()); + virginBodySending.plan(); else finishNullOrEmptyBodyPreview(httpBuf); } if (shouldAllow204()) { + debugs(93,5, HERE << "will allow 204s outside of preview"); + state.allowedPostview204 = true; buf.Printf("Allow: 204\r\n"); - // be robust: do not rely on the expected body size - if (virginBody.expected()) // there is a body to protect - virginSendClaim.protectAll(); + if (virginBody.expected()) // there is a body to echo + virginBodySending.plan(); } if (TheICAPConfig.send_client_ip && request) @@ -1135,7 +1127,7 @@ // body_pipe. So we handle preview of null-body and zero-size bodies here. void ICAPModXact::finishNullOrEmptyBodyPreview(MemBuf &buf) { - Must(!virginWriteClaim.active()); // one reason we handle it here + Must(!virginBodyWriting.active()); // one reason we handle it here Must(!virgin.body_pipe); // another reason we handle it here Must(!preview.ad()); @@ -1168,7 +1160,7 @@ buf.Printf("P(%d)", (int) preview.debt()); } - if (virginSendClaim.active()) + if (virginBodySending.active()) buf.append("B", 1); if (!state.doneParsing() && state.parsing != State::psIcapHeader) @@ -1237,11 +1229,7 @@ virgin.body_pipe << "; size: " << size); virginBody.expect(size); - - if (size < 0) // unknown size - virginWriteClaim.protectAll(); - else - virginWriteClaim.protectUpTo(size); + virginBodyWriting.plan(); // sign up as a body consumer Must(msg->body_pipe != NULL); @@ -1297,55 +1285,33 @@ -MemBufClaim::MemBufClaim(): theStart(-1), theGoal(-1) +VirginBodyAct::VirginBodyAct(): theStart(-1) {} -void MemBufClaim::protectAll() +void VirginBodyAct::plan() { if (theStart < 0) theStart = 0; - - theGoal = -1; // no specific goal } -void MemBufClaim::protectUpTo(size_t aGoal) +void VirginBodyAct::disable() { - if (theStart < 0) - theStart = 0; - - Must(aGoal >= 0); - - theGoal = (theGoal < 0) ? static_cast(aGoal) : - XMIN(static_cast(aGoal), theGoal); + theStart = -2; } -void MemBufClaim::disable() -{ - theStart = -1; -} - -void MemBufClaim::release(size_t size) +void VirginBodyAct::progress(size_t size) { Must(active()); Must(size >= 0); theStart += static_cast(size); - - if (limited() && theStart >= theGoal) - disable(); } -size_t MemBufClaim::offset() const +size_t VirginBodyAct::offset() const { Must(active()); return static_cast(theStart); } -bool MemBufClaim::limited() const -{ - Must(active()); - return theGoal >= 0; -} - ICAPPreview::ICAPPreview(): theWritten(0), theAd(0), theState(stDisabled) {} Index: squid3/src/ICAP/ICAPModXact.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPModXact.h,v retrieving revision 1.1.2.9 retrieving revision 1.1.2.10 diff -u -r1.1.2.9 -r1.1.2.10 --- squid3/src/ICAP/ICAPModXact.h 14 Feb 2007 06:10:22 -0000 1.1.2.9 +++ squid3/src/ICAP/ICAPModXact.h 20 Feb 2007 16:22:44 -0000 1.1.2.10 @@ -1,6 +1,6 @@ /* - * $Id: ICAPModXact.h,v 1.1.2.9 2007/02/14 06:10:22 rousskov Exp $ + * $Id: ICAPModXact.h,v 1.1.2.10 2007/02/20 16:22:44 rousskov Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -71,32 +71,31 @@ ssize_t theData; // combines expectation and size info to save RAM }; -// Protects buffer area. If area size is unknown, protects buffer suffix. -// Only "released" data can be consumed by the caller. Used to maintain -// write, preview, and 204 promises for ICAPModXact virgin->data-body buffer. - -class MemBufClaim +// Virgin body may be used for two activities: (a) writing preview or prime +// body to the ICAP server and (b) sending the body back in the echo mode. +// Both activities use the same BodyPipe and may be active at the same time. +// This class is used to maintain the state of body writing or sending +// activity and to coordinate consumption of the shared virgin body buffer. +class VirginBodyAct { public: - MemBufClaim(); + VirginBodyAct(); // disabled by default - void protectAll(); - void protectUpTo(size_t aGoal); - void disable(); - bool active() const { return theStart >= 0; } + void plan(); // the activity may happen; do not consume at or above offset + void disable(); // the activity wont continue; no consumption restrictions + bool active() const { return theStart >= 0; } // planned and not disabled // methods below require active() - void release(size_t size); // stop protecting size more bytes - size_t offset() const; // protected area start - bool limited() const; // protects up to a known size goal + size_t offset() const; // the absolute beginning of not-yet-acted-on data + void progress(size_t size); // note processed body bytes private: - ssize_t theStart; // left area border - ssize_t theGoal; // "end" maximum, if any + ssize_t theStart; // offset, unless negative. }; + // maintains preview-related sizes class ICAPPreview @@ -182,13 +181,12 @@ virtual bool doneReading() const { return commEof || state.doneParsing(); } virtual bool doneWriting() const { return state.doneWriting(); } - size_t claimSize(const MemBufClaim &claim) const; - const char *claimContent(const MemBufClaim &claim) const; - bool doneWithClaim(const MemBufClaim &claim) const; + size_t virginContentSize(const VirginBodyAct &act) const; + const char *virginContentData(const VirginBodyAct &act) const; + bool virginBodyEndReached(const VirginBodyAct &act) const; void makeRequestHeaders(MemBuf &buf); void makeUsernameHeader(const HttpRequest *request, MemBuf &buf); - void moveRequestChunk(MemBuf &buf, size_t chunkSize); void addLastRequestChunk(MemBuf &buf); void openChunk(MemBuf &buf, size_t chunkSize, bool ieof); void closeChunk(MemBuf &buf); @@ -243,8 +241,8 @@ HttpReply *icapReply; SizedEstimate virginBody; - MemBufClaim virginWriteClaim; // preserve virgin data buffer for writing - MemBufClaim virginSendClaim; // ... for sending (preview and 204s) + VirginBodyAct virginBodyWriting; // virgin body writing state + VirginBodyAct virginBodySending; // virgin body sending state size_t virginConsumed; // virgin data consumed so far ICAPPreview preview; // use for creating (writing) the preview @@ -259,6 +257,7 @@ public: bool serviceWaiting; // waiting for ICAP service options + bool allowedPostview204; // mmust handle 204 No Content outside preview // will not write anything [else] to the ICAP server connection bool doneWriting() const { return writing == writingReallyDone; }