--------------------- PatchSet 4390 Date: 2007/04/25 21:28:10 Author: rousskov Branch: squid3-icap Tag: (none) Log: Bug #1819 fix, part 2: retry the ICAP transaction when its pconn fails. Part 2 changes should enable Squid to handle race conditions of ICAP pconns. The changes in these files (all ICAP initiators) were limited to renaming methods and types, except the client side no longer bypasses failed queries for essential ICAP services because ICAPLauncher tells its initiator whether the failure is bypassable. The related bigger changes are described below: HTTP client, server, and ICAP server representative (all ICAPInitiators) used to start ICAP transactions. If a transaction fails due to a persistent connection race condition, the initiator would either terminate the HTTP transaction or bypass the ICAP failure, violating the protocol. Now, instead of starting an ICAP transaction directly, the above initiators start ICAP Launcher. The Launcher starts the ICAP transaction and retries it (i.e., starts another transaction with the same set of parameters) if needed. The Launcher is both ICAP initiator (because it starts ICAP transactions) and ICAP initiate (because it is started by other ICAP initiators). The launcher proxies ICAP communication (usually one or two messages each way) and makes retries transparent to initiators. All ICAP transactions corresponding to the same adaptation of an HTTP message are sometimes referred to as a single ICAP query. An ICAP transaction can be retried until it read data from the ICAP server or consumed virgin body. This means that a ICAP transaction may go as far as writing the entire ICAP Preview before aborting in a retriable state. When retrying, persistent connections are not used. If the first retry fails, the query fails. Thus, a query cannot contain more than two transactions. Two new things were added to support ICAPLauncher: First, AsyncJob, is a class that handles the basic logic of maintaining an asynchronous job or task such as an ICAP transaction or a launcher. All core AsyncJob functionality was moved from ICAPXaction and ICAPModXact classes. We could not use the two classes for the launcher because ICAPLauncher is not a transaction. While currently specific to ICAP, this object may eventually be moved to Squid core and serve as a base for other asynchronous jobs or logical threads. Second, a temporary hack was added to support cbdata operations by two parents of a single CBDATA class (ICAPLauncher is both ICAP Initiator and Initiate). The second parent (ICAPInitiator) defines toCbdata() method that ICAPLauncher overwrites to provide a usable cbdata pointer to code that only sees an ICAPInitiator pointer. The ICAPInitiator pointer is unusable for cbdata when it comes from the middle of the ICAPLauncher object. Also, the initiator pointer is stored along with its cbdata so that the latter can be accessed (and unlocked) even if the initiator object is already deleted. These hacks will disappear once cbdata becomes object-aware. Members: src/Server.cc:1.4.2.8->1.4.2.9 src/Server.h:1.1.12.6->1.1.12.7 src/client_side_request.cc:1.34.4.30->1.34.4.31 src/client_side_request.h:1.17.12.11->1.17.12.12 src/ICAP/ICAPServiceRep.cc:1.1.2.14->1.1.2.15 src/ICAP/ICAPServiceRep.h:1.1.2.10->1.1.2.11 Index: squid3/src/Server.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/Server.cc,v retrieving revision 1.4.2.8 retrieving revision 1.4.2.9 diff -u -r1.4.2.8 -r1.4.2.9 --- squid3/src/Server.cc 23 Apr 2007 20:06:32 -0000 1.4.2.8 +++ squid3/src/Server.cc 25 Apr 2007 21:28:10 -0000 1.4.2.9 @@ -1,5 +1,5 @@ /* - * $Id: Server.cc,v 1.4.2.8 2007/04/23 20:06:32 rousskov Exp $ + * $Id: Server.cc,v 1.4.2.9 2007/04/25 21:28:10 rousskov Exp $ * * DEBUG: * AUTHOR: Duane Wessels @@ -325,8 +325,8 @@ virginBodyDestination << "; size: " << size); } - adaptedHeadSource = startIcapXaction( - new ICAPModXact(this, reply, cause, service)); + adaptedHeadSource = initiateIcap( + new ICAPModXactLauncher(this, reply, cause, service)); return true; } @@ -372,7 +372,7 @@ { HttpReply *rep = dynamic_cast(msg); HTTPMSGLOCK(rep); - clearIcapXaction(adaptedHeadSource); // we do not expect more messages + clearIcap(adaptedHeadSource); // we do not expect more messages if (abortOnBadEntry("entry went bad while waiting for adapted headers")) { HTTPMSGUNLOCK(rep); // hopefully still safe, even if "this" is deleted @@ -402,10 +402,10 @@ // will not receive adapted response headers (and, hence, body) void -ServerStateData::noteIcapQueryAborted() +ServerStateData::noteIcapQueryAbort(bool final) { - clearIcapXaction(adaptedHeadSource); - handleIcapAborted(); + clearIcap(adaptedHeadSource); + handleIcapAborted(!final); } // more adapted response body is available @@ -459,13 +459,16 @@ // common part of noteIcap*Aborted and noteBodyConsumerAborted methods void -ServerStateData::handleIcapAborted() +ServerStateData::handleIcapAborted(bool bypassable) { - debugs(11,5, HERE << "handleIcapAborted; entry empty: " << entry->isEmpty()); + debugs(11,5, HERE << "handleIcapAborted; bypassable: " << bypassable << + ", entry empty: " << entry->isEmpty()); if (abortOnBadEntry("entry went bad while ICAP aborted")) return; + // TODO: bypass if possible + if (entry->isEmpty()) { debugs(11,9, HERE << "creating ICAP error entry after ICAP failure"); ErrorState *err = Index: squid3/src/Server.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/Server.h,v retrieving revision 1.1.12.6 retrieving revision 1.1.12.7 diff -u -r1.1.12.6 -r1.1.12.7 --- squid3/src/Server.h 23 Apr 2007 16:23:45 -0000 1.1.12.6 +++ squid3/src/Server.h 25 Apr 2007 21:28:10 -0000 1.1.12.7 @@ -1,6 +1,6 @@ /* - * $Id: Server.h,v 1.1.12.6 2007/04/23 16:23:45 rousskov Exp $ + * $Id: Server.h,v 1.1.12.7 2007/04/25 21:28:10 rousskov Exp $ * * AUTHOR: Duane Wessels * @@ -90,7 +90,7 @@ // ICAPInitiator: start an ICAP transaction and receive adapted headers. virtual void noteIcapAnswer(HttpMsg *message); - virtual void noteIcapQueryAborted(); + virtual void noteIcapQueryAbort(bool final); // BodyProducer: provide virgin response body to ICAP. virtual void noteMoreBodySpaceAvailable(BodyPipe &); @@ -134,7 +134,7 @@ void handleAdaptedBodyProducerAborted(); void handleIcapCompleted(); - void handleIcapAborted(); + void handleIcapAborted(bool bypassable = false); #endif public: // should not be @@ -149,7 +149,7 @@ #if ICAP_CLIENT BodyPipe::Pointer virginBodyDestination; // to provide virgin response body - ICAPXaction *adaptedHeadSource; // to get adapted response headers + ICAPInitiate *adaptedHeadSource; // to get adapted response headers BodyPipe::Pointer adaptedBodySource; // to consume adated response body bool icapAccessCheckPending; Index: squid3/src/client_side_request.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/client_side_request.cc,v retrieving revision 1.34.4.30 retrieving revision 1.34.4.31 diff -u -r1.34.4.30 -r1.34.4.31 --- squid3/src/client_side_request.cc 23 Apr 2007 17:54:23 -0000 1.34.4.30 +++ squid3/src/client_side_request.cc 25 Apr 2007 21:28:11 -0000 1.34.4.31 @@ -1,6 +1,6 @@ /* - * $Id: client_side_request.cc,v 1.34.4.30 2007/04/23 17:54:23 rousskov Exp $ + * $Id: client_side_request.cc,v 1.34.4.31 2007/04/25 21:28:11 rousskov Exp $ * * DEBUG: section 85 Client-side Request Routines * AUTHOR: Robert Collins (Originally Duane Wessels in client_side.c) @@ -1086,8 +1086,8 @@ assert(!icapHeadSource); assert(!icapBodySource); - icapHeadSource = startIcapXaction( - new ICAPModXact(this, request, NULL, service)); + icapHeadSource = initiateIcap( + new ICAPModXactLauncher(this, request, NULL, service)); return true; } @@ -1131,18 +1131,18 @@ } // we are done with getting headers (but may be receiving body) - clearIcapXaction(icapHeadSource); + clearIcap(icapHeadSource); if (!request_satisfaction_mode) doCallouts(); } void -ClientHttpRequest::noteIcapQueryAborted() +ClientHttpRequest::noteIcapQueryAbort(bool final) { - clearIcapXaction(icapHeadSource); + clearIcap(icapHeadSource); assert(!icapBodySource); - handleIcapFailure(); + handleIcapFailure(!final); } void @@ -1198,18 +1198,16 @@ } void -ClientHttpRequest::handleIcapFailure() +ClientHttpRequest::handleIcapFailure(bool bypassable) { - debugs(85,3, HERE << "handleIcapFailure"); + debugs(85,3, HERE << "handleIcapFailure(" << bypassable << ")"); const bool usedStore = storeEntry() && !storeEntry()->isEmpty(); const bool usedPipe = request->body_pipe != NULL && request->body_pipe->consumedSize() > 0; - // XXX: we must not try to recover if the ICAP service is not bypassable! - - if (!usedStore && !usedPipe) { - debug(85,2)("WARNING: ICAP REQMOD callout failed, proceeding with original request\n"); + if (bypassable && !usedStore && !usedPipe) { + debugs(85,3, HERE << "ICAP REQMOD callout failed, bypassing: " << calloutContext); if (calloutContext) doCallouts(); return; Index: squid3/src/client_side_request.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/client_side_request.h,v retrieving revision 1.17.12.11 retrieving revision 1.17.12.12 diff -u -r1.17.12.11 -r1.17.12.12 --- squid3/src/client_side_request.h 23 Apr 2007 16:23:46 -0000 1.17.12.11 +++ squid3/src/client_side_request.h 25 Apr 2007 21:28:11 -0000 1.17.12.12 @@ -1,6 +1,6 @@ /* - * $Id: client_side_request.h,v 1.17.12.11 2007/04/23 16:23:46 rousskov Exp $ + * $Id: client_side_request.h,v 1.17.12.12 2007/04/25 21:28:11 rousskov Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -162,12 +162,14 @@ public: bool startIcap(ICAPServiceRep::Pointer); - void handleIcapFailure(); // private but exposed for ClientRequestContext + + // private but exposed for ClientRequestContext + void handleIcapFailure(bool bypassable = false); private: // ICAPInitiator API, called by ICAPXaction virtual void noteIcapAnswer(HttpMsg *message); - virtual void noteIcapQueryAborted(); + virtual void noteIcapQueryAbort(bool final); // BodyConsumer API, called by BodyPipe virtual void noteMoreBodyDataAvailable(BodyPipe &); @@ -177,7 +179,7 @@ void endRequestSatisfaction(); private: - ICAPXaction *icapHeadSource; + ICAPInitiate *icapHeadSource; BodyPipe::Pointer icapBodySource; bool request_satisfaction_mode; Index: squid3/src/ICAP/ICAPServiceRep.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPServiceRep.cc,v retrieving revision 1.1.2.14 retrieving revision 1.1.2.15 diff -u -r1.1.2.14 -r1.1.2.15 --- squid3/src/ICAP/ICAPServiceRep.cc 23 Apr 2007 16:23:54 -0000 1.1.2.14 +++ squid3/src/ICAP/ICAPServiceRep.cc 25 Apr 2007 21:28:11 -0000 1.1.2.15 @@ -432,7 +432,7 @@ handleNewOptions(newOptions); } -void ICAPServiceRep::noteIcapQueryAborted() { +void ICAPServiceRep::noteIcapQueryAbort(bool) { Must(waiting); waiting = false; @@ -457,7 +457,7 @@ debugs(93,6, "ICAPService will get new options " << status()); waiting = true; - startIcapXaction(new ICAPOptXact(this, self)); + initiateIcap(new ICAPOptXactLauncher(this, self)); // TODO: timeout in case ICAPOptXact never calls us back? // Such a timeout should probably be a generic AsyncStart feature. } Index: squid3/src/ICAP/ICAPServiceRep.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPServiceRep.h,v retrieving revision 1.1.2.10 retrieving revision 1.1.2.11 diff -u -r1.1.2.10 -r1.1.2.11 --- squid3/src/ICAP/ICAPServiceRep.h 23 Apr 2007 16:23:54 -0000 1.1.2.10 +++ squid3/src/ICAP/ICAPServiceRep.h 25 Apr 2007 21:28:11 -0000 1.1.2.11 @@ -1,6 +1,6 @@ /* - * $Id: ICAPServiceRep.h,v 1.1.2.10 2007/04/23 16:23:54 rousskov Exp $ + * $Id: ICAPServiceRep.h,v 1.1.2.11 2007/04/25 21:28:11 rousskov Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -117,7 +117,7 @@ // receive either an ICAP OPTIONS response header or an abort message virtual void noteIcapAnswer(HttpMsg *msg); - virtual void noteIcapQueryAborted(); + virtual void noteIcapQueryAbort(bool); private: // stores Prepare() callback info