Sign Up
Log In
Log In
or
Sign Up
Places
All Projects
Status Monitor
Collapse sidebar
openSUSE:Evergreen:11.1:kernel-2.6.32
squid-beta
10858.patch
Overview
Repositories
Revisions
Requests
Users
Attributes
Meta
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 {
Locations
Projects
Search
Status Monitor
Help
OpenBuildService.org
Documentation
API Documentation
Code of Conduct
Contact
Support
@OBShq
Terms
openSUSE Build Service is sponsored by
The Open Build Service is an
openSUSE project
.
Sign Up
Log In
Places
Places
All Projects
Status Monitor