From 478cfe99eccf60060ca4fdec9ccdd2f37da24d4a Mon Sep 17 00:00:00 2001 From: rousskov <> Date: Wed, 20 Jun 2007 03:12:15 +0000 Subject: [PATCH] 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. --- src/ICAP/ICAPModXact.cc | 125 ++++++++++++++++++++++++++++++++++++---- src/ICAP/ICAPModXact.h | 25 ++++++-- 2 files changed, 135 insertions(+), 15 deletions(-) diff --git a/src/ICAP/ICAPModXact.cc b/src/ICAP/ICAPModXact.cc index 1732e4026f..3e8466b745 100644 --- a/src/ICAP/ICAPModXact.cc +++ b/src/ICAP/ICAPModXact.cc @@ -42,7 +42,8 @@ ICAPModXact::ICAPModXact(ICAPInitiator *anInitiator, HttpMsg *virginHeader, ICAPXaction("ICAPModXact", anInitiator, aService), icapReply(NULL), virginConsumed(0), - bodyParser(NULL) + bodyParser(NULL), + canStartBypass(false) // too early { assert(virginHeader); @@ -70,6 +71,8 @@ void ICAPModXact::start() 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 @@ void ICAPModXact::noteServiceReady() startWriting(); } else { disableRetries(); - mustStop("ICAP service unusable"); + throw TexcHere("ICAP service is unusable"); } ICAPXaction_Exit(); @@ -348,6 +351,8 @@ const char *ICAPModXact::virginContentData(const VirginBodyAct &act) const void ICAPModXact::virginConsume() { + debugs(93, 9, "consumption guards: " << !virgin.body_pipe << isRetriable); + if (!virgin.body_pipe) return; // nothing to consume @@ -355,10 +360,28 @@ void ICAPModXact::virginConsume() 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(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 @@ void ICAPModXact::virginConsume() bp.consume(size); virginConsumed += size; Must(!isRetriable); // or we should not be consuming + disableBypass("consumed content"); } } @@ -404,15 +428,16 @@ void ICAPModXact::stopWriting(bool nicely) // 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 @@ void ICAPModXact::echoMore() " bytes"); virginBodySending.progress(size); virginConsume(); + disableBypass("echoed content"); } if (virginBodyEndReached(virginBodySending)) { @@ -507,6 +533,7 @@ bool ICAPModXact::doneSending() const 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 @@ void ICAPModXact::parseMore() 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 @@ void ICAPModXact::parseHeaders() 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::handle200Ok() 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 @@ void ICAPModXact::handle204NoContent() 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 @@ void ICAPModXact::parseBody() 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 @@ void ICAPModXact::fillPendingStatus(MemBuf &buf) const 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 @@ size_t SizedEstimate::size() const -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(size); + theStart += size; } size_t VirginBodyAct::offset() const { Must(active()); - return static_cast(theStart); + return theStart; } diff --git a/src/ICAP/ICAPModXact.h b/src/ICAP/ICAPModXact.h index d2739d8af7..e80729970e 100644 --- a/src/ICAP/ICAPModXact.h +++ b/src/ICAP/ICAPModXact.h @@ -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 @@ class VirginBodyAct { 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 @@ public: 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 @@ public: 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 @@ private: 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 @@ private: ChunkedCodingParser *bodyParser; // ICAP response body parser + bool canStartBypass; // enables bypass of transaction failures + class State { -- 2.39.5