--------------------- PatchSet 4441 Date: 2007/05/02 16:53:25 Author: rousskov Branch: squid3-icap Tag: (none) Log: - Preserve virgin body backup if we decided to retry the transaction on pconn failures. The old code would release backup if there is no preview and no 204s are allowed, but transactions without preview and 204 may still be retried if we can backup the entire body. Preview and 204 depend, in part, on the ICAP server while "retriability" does not. - Decide on preview before connecting to the ICAP server because if we are doing preview, we can retry a failed pconn even if we cannot backup the entire body. Renamed shouldPreview() to decideOnPreview() and made the call less dependent on the context. - Added decideOnRetries() to disable pconn retries if needed. Retries may be enabled even though preview and 204 are not allowed because preview and 204 depend, in part, on the ICAP server while "retriability" does not. Members: src/ICAP/ICAPModXact.cc:1.1.2.28->1.1.2.29 src/ICAP/ICAPModXact.h:1.1.2.13->1.1.2.14 Index: squid3/src/ICAP/ICAPModXact.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPModXact.cc,v retrieving revision 1.1.2.28 retrieving revision 1.1.2.29 diff -u -r1.1.2.28 -r1.1.2.29 --- squid3/src/ICAP/ICAPModXact.cc 25 Apr 2007 22:03:10 -0000 1.1.2.28 +++ squid3/src/ICAP/ICAPModXact.cc 2 May 2007 16:53:25 -0000 1.1.2.29 @@ -118,6 +118,10 @@ void ICAPModXact::startWriting() { state.writing = State::writingConnect; + + decideOnPreview(); // must be decided before we decideOnRetries + decideOnRetries(); + openConnection(); // put nothing here as openConnection calls commConnectStart // and that may call us back without waiting for the next select loop @@ -345,7 +349,10 @@ void ICAPModXact::virginConsume() { if (!virgin.body_pipe) - return; + return; // nothing to consume + + if (isRetriable) + return; // do not consume if we may have to retry later BodyPipe &bp = *virgin.body_pipe; const size_t have = static_cast(bp.buf().contentSize()); @@ -365,7 +372,7 @@ " virgin body bytes"); bp.consume(size); virginConsumed += size; - disableRetries(); + Must(!isRetriable); // or we should not be consuming } } @@ -1011,7 +1018,7 @@ buf.append(ICAP::crlf, 2); // terminate Encapsulated line - if (shouldPreview(urlPath)) { + if (preview.enabled()) { buf.Printf("Preview: %d\r\n", (int)preview.ad()); if (virginBody.expected()) // there is a body to preview virginBodySending.plan(); @@ -1074,20 +1081,25 @@ } // decides whether to offer a preview and calculates its size -bool ICAPModXact::shouldPreview(const String &urlPath) +void ICAPModXact::decideOnPreview() { if (!TheICAPConfig.preview_enable) { debugs(93, 5, HERE << "preview disabled by squid.conf"); - return false; + return; } + const HttpRequest *request = virgin.cause ? + virgin.cause : + dynamic_cast(virgin.header); + const String urlPath = request ? request->urlpath : String(); size_t wantedSize; - if (!service().wantsPreview(urlPath, wantedSize)) { debugs(93, 5, "ICAPModXact should not offer preview for " << urlPath); - return false; + return; } + // we decided to do preview, now compute its size + Must(wantedSize >= 0); // cannot preview more than we can backup @@ -1104,8 +1116,6 @@ preview.enable(ad); Must(preview.enabled()); - - return true; } // decides whether to allow 204 responses @@ -1114,10 +1124,16 @@ if (!service().allows204()) return false; + return canBackupEverything(); +} + +// used by shouldAllow204 and decideOnRetries +bool ICAPModXact::canBackupEverything() const +{ if (!virginBody.expected()) - return true; // no body means no problems with supporting 204s. + return true; // no body means no problems with backup - // if there is a body, make sure we can backup it all + // if there is a body, check whether we can backup it all if (!virginBody.knownSize()) return false; @@ -1127,6 +1143,22 @@ return virginBody.size() < TheBackupLimit; } +// Decide whether this transaction can be retried if pconn fails +// Must be called after decideOnPreview and before openConnection() +void ICAPModXact::decideOnRetries() +{ + if (!isRetriable) + return; // no, already decided + + if (preview.enabled()) + return; // yes, because preview provides enough guarantees + + if (canBackupEverything()) + return; // yes, because we can back everything up + + disableRetries(); // no, because we cannot back everything up +} + // Normally, the body-writing code handles preview body. It can deal with // bodies of unexpected size, including those that turn out to be empty. // However, that code assumes that the body was expected and body control Index: squid3/src/ICAP/ICAPModXact.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPModXact.h,v retrieving revision 1.1.2.13 retrieving revision 1.1.2.14 diff -u -r1.1.2.13 -r1.1.2.14 --- squid3/src/ICAP/ICAPModXact.h 25 Apr 2007 21:23:50 -0000 1.1.2.13 +++ squid3/src/ICAP/ICAPModXact.h 2 May 2007 16:53:25 -0000 1.1.2.14 @@ -1,6 +1,6 @@ /* - * $Id: ICAPModXact.h,v 1.1.2.13 2007/04/25 21:23:50 rousskov Exp $ + * $Id: ICAPModXact.h,v 1.1.2.14 2007/05/02 16:53:25 rousskov Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -187,8 +187,11 @@ void virginConsume(); void finishNullOrEmptyBodyPreview(MemBuf &buf); - bool shouldPreview(const String &urlPath); + void decideOnPreview(); + void decideOnRetries(); bool shouldAllow204(); + bool canBackupEverything() const; + void prepBackup(size_t expectedSize); void backup(const MemBuf &buf);