File 10858.patch of Package squid-beta
---------------------
PatchSet 10858
Date: 2007/06/19 21:12:15
Author: rousskov
Branch: HEAD
Tag: (none)
Log:
Bug #1974 fix: Bypass failures of optional ICAP services.
To bypass various failures, ICAPModXact catches its own exceptions and enables
the "echo" mode instead of quitting. Exceptions are not overruled if the
transaction is retriable. The code disables bypass for essential ICAP services
and when something makes clean echoing impossible (e.g., some buffered HTTP
body content was consumed).
This design allows the same bypass mechanism to be used for moth REQMOD and
RESPMOD, regardless of the vectoring point. Its implementation did not require
changing any Squid core files.
Previously many if not most ICAP failures were not bypassed and users were
receiving cryptic "ICAP protocol error" messages when an optional ICAP
service went down. With the current code, the service may go up and down many
times but only the transactions with large message bodies (that were executing
when the service went down) should be affected.
When deciding on whether to consume HTTP content written to the ICAP server,
Squid has to resolve a trade-off: postponing consumption longer increases
process footprint and may slow the HTTP side down, but consuming sooner
increases the chance of that "ICAP protocol" error being returned to the user.
Since Squid cannot buffer very large messages, some errors are inevitable. We
may want to add knobs to control this tradeoff.
The entries below are only indirectly related to the bug #1974 fix.
virginConsume() does not call or imply checkConsuming(). We must call
checkConsuming() even if we called virginConsume(). Otherwise, we may leave
and get destroyed while the pipe object still holds a [consumer] pointer to
us.
Polished VirginBodyAct so that we can distinguish disabled from undecided
states without using magical negative size constants.
Do not stop writing before throwing an exception. If the exception kills the
transaction, the transaction should cleanup the writer anyway. To bypass an
exception, we need all virgin content intact. Stopping writes before throwing
an exception may consume virgin content because the code does not know that
the exception is about to be thrown and may perceive content as "no longer
needed".
Added debugging.
Merged from the squid3-icap branch.
Members:
src/ICAP/ICAPModXact.cc:1.33->1.34
src/ICAP/ICAPModXact.h:1.8->1.9
Index: squid3/src/ICAP/ICAPModXact.cc
===================================================================
RCS file: /cvsroot/squid/squid3/src/ICAP/ICAPModXact.cc,v
retrieving revision 1.33
retrieving revision 1.34
diff -u -r1.33 -r1.34
--- squid3/src/ICAP/ICAPModXact.cc 29 May 2007 13:31:44 -0000 1.33
+++ squid3/src/ICAP/ICAPModXact.cc 19 Jun 2007 21:12:15 -0000 1.34
@@ -42,7 +42,8 @@
ICAPXaction("ICAPModXact", anInitiator, aService),
icapReply(NULL),
virginConsumed(0),
- bodyParser(NULL)
+ bodyParser(NULL),
+ canStartBypass(false) // too early
{
assert(virginHeader);
@@ -70,6 +71,8 @@
estimateVirginBody(); // before virgin disappears!
+ canStartBypass = service().bypass;
+
// it is an ICAP violation to send request to a service w/o known OPTIONS
if (service().up())
@@ -109,7 +112,7 @@
startWriting();
} else {
disableRetries();
- mustStop("ICAP service unusable");
+ throw TexcHere("ICAP service is unusable");
}
ICAPXaction_Exit();
@@ -348,6 +351,8 @@
void ICAPModXact::virginConsume()
{
+ debugs(93, 9, "consumption guards: " << !virgin.body_pipe << isRetriable);
+
if (!virgin.body_pipe)
return; // nothing to consume
@@ -355,10 +360,28 @@
return; // do not consume if we may have to retry later
BodyPipe &bp = *virgin.body_pipe;
+
+ // Why > 2? HttpState does not use the last bytes in the buffer
+ // because delayAwareRead() is arguably broken. See
+ // HttpStateData::maybeReadVirginBody for more details.
+ if (canStartBypass && bp.buf().spaceSize() > 2) {
+ // Postponing may increase memory footprint and slow the HTTP side
+ // down. Not postponing may increase the number of ICAP errors
+ // if the ICAP service fails. We may also use "potential" space to
+ // postpone more aggressively. Should the trade-off be configurable?
+ debugs(93, 8, HERE << "postponing consumption from " << bp.status());
+ return;
+ }
+
const size_t have = static_cast<size_t>(bp.buf().contentSize());
const size_t end = virginConsumed + have;
size_t offset = end;
+ debugs(93, 9, HERE << "max virgin consumption offset=" << offset <<
+ " acts " << virginBodyWriting.active() << virginBodySending.active() <<
+ " consumed=" << virginConsumed <<
+ " from " << virgin.body_pipe->status());
+
if (virginBodyWriting.active())
offset = XMIN(virginBodyWriting.offset(), offset);
@@ -373,6 +396,7 @@
bp.consume(size);
virginConsumed += size;
Must(!isRetriable); // or we should not be consuming
+ disableBypass("consumed content");
}
}
@@ -404,15 +428,16 @@
// call at any time, usually in the middle of the destruction sequence!
// Somebody should add comm_remove_write_handler() to comm API.
reuseConnection = false;
+ ignoreLastWrite = true;
}
debugs(93, 7, HERE << "will no longer write" << status());
- state.writing = State::writingReallyDone;
-
if (virginBodyWriting.active()) {
virginBodyWriting.disable();
virginConsume();
}
+ state.writing = State::writingReallyDone;
+ checkConsuming();
}
void ICAPModXact::stopBackup()
@@ -489,6 +514,7 @@
" bytes");
virginBodySending.progress(size);
virginConsume();
+ disableBypass("echoed content");
}
if (virginBodyEndReached(virginBodySending)) {
@@ -507,6 +533,7 @@
return state.sending == State::sendingDone;
}
+// stop (or do not start) sending adapted message body
void ICAPModXact::stopSending(bool nicely)
{
if (doneSending())
@@ -554,6 +581,56 @@
parseBody();
}
+void ICAPModXact::callException(const TextException &e)
+{
+ if (!canStartBypass || isRetriable) {
+ ICAPXaction::callException(e);
+ return;
+ }
+
+ try {
+ debugs(93, 2, "bypassing ICAPModXact::" << inCall << " exception: " <<
+ e.message << ' ' << status());
+ bypassFailure();
+ }
+ catch (const TextException &bypassE) {
+ ICAPXaction::callException(bypassE);
+ }
+}
+
+void ICAPModXact::bypassFailure()
+{
+ disableBypass("already started to bypass");
+
+ Must(!isRetriable); // or we should not be bypassing
+
+ prepEchoing();
+
+ startSending();
+
+ // end all activities associated with the ICAP server
+
+ stopParsing();
+
+ stopWriting(true); // or should we force it?
+ if (connection >= 0) {
+ reuseConnection = false; // be conservative
+ cancelRead(); // may not work; and we cannot stop connecting either
+ if (!doneWithIo())
+ debugs(93, 7, "Warning: bypass failed to stop I/O" << status());
+ }
+}
+
+void ICAPModXact::disableBypass(const char *reason)
+{
+ if (canStartBypass) {
+ debugs(93,7, HERE << "will never start bypass because " << reason);
+ canStartBypass = false;
+ }
+}
+
+
+
// note that allocation for echoing is done in handle204NoContent()
void ICAPModXact::maybeAllocateHttpMsg()
{
@@ -587,6 +664,13 @@
return;
}
+ startSending();
+}
+
+// called after parsing all headers or when bypassing an exception
+void ICAPModXact::startSending()
+{
+ disableBypass("sent headers");
sendAnswer(adapted.header);
if (state.sending == State::sendingVirgin)
@@ -687,6 +771,15 @@
void ICAPModXact::handle204NoContent()
{
stopParsing();
+ prepEchoing();
+}
+
+// Called when we receive a 204 No Content response and
+// when we are trying to bypass a service failure.
+// We actually start sending (echoig or not) in startSending.
+void ICAPModXact::prepEchoing()
+{
+ disableBypass("preparing to echo content");
// We want to clone the HTTP message, but we do not want
// to copy some non-HTTP state parts that HttpMsg kids carry in them.
@@ -732,9 +825,11 @@
if (oldHead->body_pipe != NULL) {
debugs(93, 7, HERE << "will echo virgin body from " <<
oldHead->body_pipe);
+ if (!virginBodySending.active())
+ virginBodySending.plan(); // will throw if not possible
state.sending = State::sendingVirgin;
checkConsuming();
- 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");
@@ -850,6 +945,10 @@
debugs(93, 5, HERE << "have " << readBuf.contentSize() << " body bytes after " <<
"parse; parsed all: " << parsed);
+ // TODO: expose BodyPipe::putSize() to make this check simpler and clearer
+ if (adapted.body_pipe->buf().contentSize() > 0) // parsed something sometime
+ disableBypass("sent adapted content");
+
if (parsed) {
stopParsing();
stopSending(true); // the parser succeeds only if all parsed data fits
@@ -1208,6 +1307,9 @@
if (!doneSending() && state.sending != State::sendingUndecided)
buf.Printf("S(%d)", state.sending);
+
+ if (canStartBypass)
+ buf.append("Y", 1);
}
void ICAPModXact::fillDoneStatus(MemBuf &buf) const
@@ -1325,31 +1427,32 @@
-VirginBodyAct::VirginBodyAct(): theStart(-1)
+VirginBodyAct::VirginBodyAct(): theStart(0), theState(stUndecided)
{}
void VirginBodyAct::plan()
{
- if (theStart < 0)
- theStart = 0;
+ Must(!disabled());
+ Must(!theStart); // not started
+ theState = stActive;
}
void VirginBodyAct::disable()
{
- theStart = -2;
+ theState = stDisabled;
}
void VirginBodyAct::progress(size_t size)
{
Must(active());
Must(size >= 0);
- theStart += static_cast<ssize_t>(size);
+ theStart += size;
}
size_t VirginBodyAct::offset() const
{
Must(active());
- return static_cast<size_t>(theStart);
+ return theStart;
}
Index: squid3/src/ICAP/ICAPModXact.h
===================================================================
RCS file: /cvsroot/squid/squid3/src/ICAP/ICAPModXact.h,v
retrieving revision 1.8
retrieving revision 1.9
diff -u -r1.8 -r1.9
--- squid3/src/ICAP/ICAPModXact.h 8 May 2007 16:32:11 -0000 1.8
+++ squid3/src/ICAP/ICAPModXact.h 19 Jun 2007 21:12:15 -0000 1.9
@@ -1,6 +1,6 @@
/*
- * $Id: ICAPModXact.h,v 1.8 2007/05/08 16:32:11 rousskov Exp $
+ * $Id: ICAPModXact.h,v 1.9 2007/06/19 21:12:15 rousskov Exp $
*
*
* SQUID Web Proxy Cache http://www.squid-cache.org/
@@ -81,11 +81,13 @@
{
public:
- VirginBodyAct(); // disabled by default
+ VirginBodyAct();
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
+
+ bool active() const { return theState == stActive; }
+ bool disabled() const { return theState == stDisabled; }
// methods below require active()
@@ -93,7 +95,10 @@
void progress(size_t size); // note processed body bytes
private:
- ssize_t theStart; // offset, unless negative.
+ size_t theStart; // unprocessed virgin body data offset
+
+ typedef enum { stUndecided, stActive, stDisabled } State;
+ State theState;
};
@@ -153,6 +158,10 @@
ICAPInOut virgin;
ICAPInOut adapted;
+protected:
+ // bypasses exceptions if needed and possible
+ virtual void callException(const TextException &e);
+
private:
virtual void start();
@@ -213,6 +222,12 @@
void handle204NoContent();
void handleUnknownScode();
+ void bypassFailure();
+
+ void startSending();
+ void disableBypass(const char *reason);
+
+ void prepEchoing();
void echoMore();
virtual bool doneAll() const;
@@ -245,6 +260,8 @@
ChunkedCodingParser *bodyParser; // ICAP response body parser
+ bool canStartBypass; // enables bypass of transaction failures
+
class State
{
