--------------------- PatchSet 4340 Date: 2007/04/23 16:23:43 Author: rousskov Branch: squid3-icap Tag: (none) Log: Bug #1819 fix, part 1: simplify ICAPInitiator-ICAPXaction link. Part 1 changes should have no effect on Squid functionality. To retry failing persistent ICAP connections we may need to start more than one ICAP transaction for a given HTTP message. This needs to be done somewhere between ICAPInitiator and ICAPXaction so that neither is affected much. The design will be similar to HTTP FwdState. This change reduces ICAPInitiator dependency on ICAPXaction so that it is easier to insert transaction retrying logic/code between them. It should have no effect on Squid functionality. HTTP client and server (both ICAPInitiators) used to keep a Pointer to ICAP transaction and extracted adapted headers from the finished transaction using that Pointer (which forced ICAPModXact to use a self-Pointer). ICAP service representative used custom callbacks to receive new options from ICAPOptXact. Now, all ICAP initiators use the same asynchronous message-based API to communicate with ICAP transactions they initiate. On the core side, the API is defined by ICAPInitiator, as before. On the ICAP side, the API is defined by the newly added ICAPInitiate class. The latter will also be used as a base for ICAPLauncher, the future ICAP transaction retrying class. Members: src/Makefile.am:1.60.4.25->1.60.4.26 src/Server.cc:1.4.2.6->1.4.2.7 src/Server.h:1.1.12.5->1.1.12.6 src/client_side_request.cc:1.34.4.28->1.34.4.29 src/client_side_request.h:1.17.12.10->1.17.12.11 src/ICAP/ICAPInitiate.cc:1.1->1.1.2.1 src/ICAP/ICAPInitiate.h:1.1->1.1.2.1 src/ICAP/ICAPInitiator.cc:1.1.2.1->1.1.2.2 src/ICAP/ICAPInitiator.h:1.1.2.1->1.1.2.2 src/ICAP/ICAPModXact.cc:1.1.2.25->1.1.2.26 src/ICAP/ICAPModXact.h:1.1.2.11->1.1.2.12 src/ICAP/ICAPOptXact.cc:1.1.2.7->1.1.2.8 src/ICAP/ICAPOptXact.h:1.1.2.4->1.1.2.5 src/ICAP/ICAPServiceRep.cc:1.1.2.13->1.1.2.14 src/ICAP/ICAPServiceRep.h:1.1.2.9->1.1.2.10 src/ICAP/ICAPXaction.cc:1.1.2.12->1.1.2.13 src/ICAP/ICAPXaction.h:1.1.2.7->1.1.2.8 Index: squid3/src/Makefile.am =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/Makefile.am,v retrieving revision 1.60.4.25 retrieving revision 1.60.4.26 diff -u -r1.60.4.25 -r1.60.4.26 --- squid3/src/Makefile.am 16 Apr 2007 19:47:22 -0000 1.60.4.25 +++ squid3/src/Makefile.am 23 Apr 2007 16:23:43 -0000 1.60.4.26 @@ -1,7 +1,7 @@ # # Makefile for the Squid Object Cache server # -# $Id: Makefile.am,v 1.60.4.25 2007/04/16 19:47:22 rousskov Exp $ +# $Id: Makefile.am,v 1.60.4.26 2007/04/23 16:23:43 rousskov Exp $ # # Uncomment and customize the following to suit your needs: # @@ -673,6 +673,8 @@ ICAP/ICAPClient.h \ ICAP/ICAPInitiator.cc \ ICAP/ICAPInitiator.h \ + ICAP/ICAPInitiate.cc \ + ICAP/ICAPInitiate.h \ ICAP/ICAPInOut.h \ ICAP/ICAPConfig.cc \ ICAP/ICAPConfig.h \ Index: squid3/src/Server.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/Server.cc,v retrieving revision 1.4.2.6 retrieving revision 1.4.2.7 diff -u -r1.4.2.6 -r1.4.2.7 --- squid3/src/Server.cc 8 Mar 2007 22:09:35 -0000 1.4.2.6 +++ squid3/src/Server.cc 23 Apr 2007 16:23:45 -0000 1.4.2.7 @@ -1,5 +1,5 @@ /* - * $Id: Server.cc,v 1.4.2.6 2007/03/08 22:09:35 rousskov Exp $ + * $Id: Server.cc,v 1.4.2.7 2007/04/23 16:23:45 rousskov Exp $ * * DEBUG: * AUTHOR: Duane Wessels @@ -39,6 +39,9 @@ #include "HttpReply.h" #include "errorpage.h" +#if ICAP_CLIENT +#include "ICAP/ICAPModXact.h" +#endif ServerStateData::ServerStateData(FwdState *theFwdState): requestSender(NULL) { @@ -283,7 +286,7 @@ } } -// called by noteIcapHeadersAdapted(), HTTP server overwrites this +// called by noteIcapAnswer(), HTTP server overwrites this void ServerStateData::haveParsedReplyHeaders() { @@ -322,8 +325,8 @@ virginBodyDestination << "; size: " << size); } - adaptedHeadSource = new ICAPModXact(this, reply, cause, service); - ICAPModXact::AsyncStart(adaptedHeadSource.getRaw()); + adaptedHeadSource = startIcapXaction( + new ICAPModXact(this, reply, cause, service)); return true; } @@ -335,10 +338,7 @@ if (virginBodyDestination != NULL) stopProducingFor(virginBodyDestination, false); - if (adaptedHeadSource != NULL) { - AsyncCall(11,5, adaptedHeadSource.getRaw(), ICAPModXact::noteInitiatorAborted); - adaptedHeadSource = NULL; - } + announceInitiatorAbort(adaptedHeadSource); if (adaptedBodySource != NULL) stopConsumingFrom(adaptedBodySource); @@ -368,12 +368,11 @@ // received adapted response headers (body may follow) void -ServerStateData::noteIcapHeadersAdapted() +ServerStateData::noteIcapAnswer(HttpMsg *msg) { - // extract and lock reply before (adaptedHeadSource = NULL) can destroy it - HttpReply *rep = dynamic_cast(adaptedHeadSource->adapted.header); + HttpReply *rep = dynamic_cast(msg); HTTPMSGLOCK(rep); - adaptedHeadSource = NULL; // we do not expect any more messages from it + clearIcapXaction(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 @@ -403,9 +402,9 @@ // will not receive adapted response headers (and, hence, body) void -ServerStateData::noteIcapHeadersAborted() +ServerStateData::noteIcapQueryAborted() { - adaptedHeadSource = NULL; + clearIcapXaction(adaptedHeadSource); handleIcapAborted(); } @@ -448,7 +447,7 @@ handleIcapAborted(); } -// common part of noteIcapHeadersAdapted and handleAdaptedBodyProductionEnded +// common part of noteIcapAnswer and handleAdaptedBodyProductionEnded void ServerStateData::handleIcapCompleted() { Index: squid3/src/Server.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/Server.h,v retrieving revision 1.1.12.5 retrieving revision 1.1.12.6 diff -u -r1.1.12.5 -r1.1.12.6 --- squid3/src/Server.h 14 Feb 2007 06:33:46 -0000 1.1.12.5 +++ squid3/src/Server.h 23 Apr 2007 16:23:45 -0000 1.1.12.6 @@ -1,6 +1,6 @@ /* - * $Id: Server.h,v 1.1.12.5 2007/02/14 06:33:46 rousskov Exp $ + * $Id: Server.h,v 1.1.12.6 2007/04/23 16:23:45 rousskov Exp $ * * AUTHOR: Duane Wessels * @@ -53,7 +53,6 @@ #if ICAP_CLIENT #include "ICAP/ICAPServiceRep.h" #include "ICAP/ICAPInitiator.h" -#include "ICAP/ICAPModXact.h" class ICAPAccessCheck; #endif @@ -90,8 +89,8 @@ virtual void icapAclCheckDone(ICAPServiceRep::Pointer) = 0; // ICAPInitiator: start an ICAP transaction and receive adapted headers. - virtual void noteIcapHeadersAdapted(); - virtual void noteIcapHeadersAborted(); + virtual void noteIcapAnswer(HttpMsg *message); + virtual void noteIcapQueryAborted(); // BodyProducer: provide virgin response body to ICAP. virtual void noteMoreBodySpaceAvailable(BodyPipe &); @@ -150,7 +149,7 @@ #if ICAP_CLIENT BodyPipe::Pointer virginBodyDestination; // to provide virgin response body - ICAPModXact::Pointer adaptedHeadSource; // to get adapted response headers + ICAPXaction *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.28 retrieving revision 1.34.4.29 diff -u -r1.34.4.28 -r1.34.4.29 --- squid3/src/client_side_request.cc 3 Apr 2007 15:10:32 -0000 1.34.4.28 +++ squid3/src/client_side_request.cc 23 Apr 2007 16:23:45 -0000 1.34.4.29 @@ -1,6 +1,6 @@ /* - * $Id: client_side_request.cc,v 1.34.4.28 2007/04/03 15:10:32 rousskov Exp $ + * $Id: client_side_request.cc,v 1.34.4.29 2007/04/23 16:23:45 rousskov Exp $ * * DEBUG: section 85 Client-side Request Routines * AUTHOR: Robert Collins (Originally Duane Wessels in client_side.c) @@ -257,10 +257,8 @@ freeResources(); #if ICAP_CLIENT - if (icapHeadSource != NULL) { - icapHeadSource->noteInitiatorAborted(); - icapHeadSource = NULL; - } + announceInitiatorAbort(icapHeadSource); + if (icapBodySource != NULL) stopConsumingFrom(icapBodySource); #endif @@ -1088,17 +1086,15 @@ assert(!icapHeadSource); assert(!icapBodySource); - icapHeadSource = new ICAPModXact(this, request, NULL, service); - ICAPModXact::AsyncStart(icapHeadSource.getRaw()); + icapHeadSource = startIcapXaction( + new ICAPModXact(this, request, NULL, service)); return true; } void -ClientHttpRequest::noteIcapHeadersAdapted() +ClientHttpRequest::noteIcapAnswer(HttpMsg *msg) { assert(cbdataReferenceValid(this)); // indicates bug - - HttpMsg *msg = icapHeadSource->adapted.header; assert(msg); if (HttpRequest *new_req = dynamic_cast(msg)) { @@ -1135,16 +1131,16 @@ } // we are done with getting headers (but may be receiving body) - icapHeadSource = NULL; + clearIcapXaction(icapHeadSource); if (!request_satisfaction_mode) doCallouts(); } void -ClientHttpRequest::noteIcapHeadersAborted() +ClientHttpRequest::noteIcapQueryAborted() { - icapHeadSource = NULL; + clearIcapXaction(icapHeadSource); assert(!icapBodySource); handleIcapFailure(); } Index: squid3/src/client_side_request.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/client_side_request.h,v retrieving revision 1.17.12.10 retrieving revision 1.17.12.11 diff -u -r1.17.12.10 -r1.17.12.11 --- squid3/src/client_side_request.h 21 Mar 2007 20:05:15 -0000 1.17.12.10 +++ squid3/src/client_side_request.h 23 Apr 2007 16:23:46 -0000 1.17.12.11 @@ -1,6 +1,6 @@ /* - * $Id: client_side_request.h,v 1.17.12.10 2007/03/21 20:05:15 rousskov Exp $ + * $Id: client_side_request.h,v 1.17.12.11 2007/04/23 16:23:46 rousskov Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -45,7 +45,6 @@ #if ICAP_CLIENT #include "ICAP/ICAPServiceRep.h" #include "ICAP/ICAPInitiator.h" -#include "ICAP/ICAPModXact.h" class HttpMsg; #endif @@ -167,8 +166,8 @@ private: // ICAPInitiator API, called by ICAPXaction - virtual void noteIcapHeadersAdapted(); - virtual void noteIcapHeadersAborted(); + virtual void noteIcapAnswer(HttpMsg *message); + virtual void noteIcapQueryAborted(); // BodyConsumer API, called by BodyPipe virtual void noteMoreBodyDataAvailable(BodyPipe &); @@ -178,7 +177,7 @@ void endRequestSatisfaction(); private: - ICAPModXact::Pointer icapHeadSource; + ICAPXaction *icapHeadSource; BodyPipe::Pointer icapBodySource; bool request_satisfaction_mode; --- /dev/null Wed May 2 00:19:29 2007 +++ squid3/src/ICAP/ICAPInitiate.cc Wed May 2 00:19:29 2007 @@ -0,0 +1,159 @@ +/* + * DEBUG: section 93 ICAP (RFC 3507) Client + */ + +#include "squid.h" +#include "HttpMsg.h" +#include "ICAPInitiator.h" +#include "ICAPInitiate.h" + +/* Event data and callback wrapper to call noteIcapAnswer with + * the adapted headers as a parameter. + * + * The call object is not cbdata-protected or refcounted because nobody + * holds a pointer to it except for the event queue. The call does check + * the Initiator pointer to see if that is still valid. + * + * TODO: convert this to a generic AsyncCall1 class + * TODO: mempool this class + */ +class ICAPAnswerCall { +public: + // use this function to make an asynchronous call: + static void Schedule(ICAPInitiator *anInitiator, HttpMsg *aMessage); + + static void Wrapper(void *data); + +protected: + ICAPAnswerCall(ICAPInitiator *anInitiator, HttpMsg *aMessage); + ~ICAPAnswerCall(); + + void schedule(); + void call(); + + ICAPInitiator *theInitiator; + HttpMsg *theMessage; +}; + + +/* ICAPInitiate */ + +ICAPInitiate::ICAPInitiate(ICAPInitiator *anInitiator, ICAPServiceRep::Pointer &aService): + theInitiator(cbdataReference(anInitiator)), theService(aService) +{ + assert(theService != NULL); + debugs(93,7, "ICAPInitiate initialized." << status()); +} + +ICAPInitiate::~ICAPInitiate() +{ + assert(!theInitiator); +} + +// internal cleanup +void ICAPInitiate::swanSong() +{ + debugs(93, 5, HERE << "swan sings" << status()); + + if (theInitiator) { + debugs(93, 3, HERE << "probably failed; sending abort notification"); + tellQueryAborted(); + } + + debugs(93, 5, HERE << "swan sang" << status()); +} + +void ICAPInitiate::clearInitiator() +{ + cbdataReferenceDone(theInitiator); +} + +void ICAPInitiate::sendAnswer(HttpMsg *msg) +{ + ICAPAnswerCall::Schedule(theInitiator, msg); + clearInitiator(); +} + +void ICAPInitiate::tellQueryAborted() +{ + AsyncCall(93,5, theInitiator, ICAPInitiator::noteIcapQueryAborted); + clearInitiator(); +} + +ICAPServiceRep &ICAPInitiate::service() +{ + assert(theService != NULL); + return *theService; +} + +const char *ICAPInitiate::status() const { + return ""; // for now +} + + +/* ICAPAnswerCall */ + +ICAPAnswerCall::ICAPAnswerCall(ICAPInitiator *anInitiator, HttpMsg *aMessage): + theInitiator(0), theMessage(0) +{ + if (anInitiator && cbdataReferenceValid(anInitiator)) { + theInitiator = cbdataReference(anInitiator); + assert(aMessage); + theMessage = HTTPMSGLOCK(aMessage); + } +} + +void ICAPAnswerCall::schedule() +{ + if (theInitiator) { + debugs(93,3, __FILE__ << "(" << __LINE__ << ") will call " << + theInitiator << "->ICAPInitiator::noteIcapAnswer(" << + theMessage << ")"); + eventAdd("ICAPInitiator::noteIcapAnswer", + &ICAPAnswerCall::Wrapper, this, 0.0, 0, false); + } else { + debugs(93,3, __FILE__ << "(" << __LINE__ << ") will not call " << + theInitiator << "->ICAPInitiator::noteIcapAnswer(" << + theMessage << ")"); + } +} + +ICAPAnswerCall::~ICAPAnswerCall() +{ + if (theInitiator) { + cbdataReferenceDone(theInitiator); + HTTPMSGUNLOCK(theMessage); + } +} + +void ICAPAnswerCall::Wrapper(void *data) +{ + assert(data); + ICAPAnswerCall *c = static_cast(data); + c->call(); + delete c; +} + +void ICAPAnswerCall::call() { + assert(theInitiator); + if (cbdataReferenceValid(theInitiator)) { + debugs(93, 3, "entering " << + theInitiator << "->ICAPInitiator::noteIcapAnswer(" << + theMessage << ")"); + theInitiator->noteIcapAnswer(theMessage); + debugs(93, 3, "exiting " << + theInitiator << "->ICAPInitiator::noteIcapAnswer(" << + theMessage << ")"); + } else { + debugs(93, 3, "ignoring " << + theInitiator << "->ICAPInitiator::noteIcapAnswer(" << + theMessage << ")"); + } +} + +void ICAPAnswerCall::Schedule(ICAPInitiator *anInitiator, HttpMsg *aMessage) +{ + ICAPAnswerCall *call = new ICAPAnswerCall(anInitiator, aMessage); + call->schedule(); + // The call object is deleted in ICAPAnswerCall::Wrapper +} --- /dev/null Wed May 2 00:19:29 2007 +++ squid3/src/ICAP/ICAPInitiate.h Wed May 2 00:19:29 2007 @@ -0,0 +1,91 @@ + +/* + * $Id: ICAPInitiate.h,v 1.1.2.1 2007/04/23 16:23:52 rousskov Exp $ + * + * + * SQUID Web Proxy Cache http://www.squid-cache.org/ + * ---------------------------------------------------------- + * + * Squid is the result of efforts by numerous individuals from + * the Internet community; see the CONTRIBUTORS file for full + * details. Many organizations have provided support for Squid's + * development; see the SPONSORS file for full details. Squid is + * Copyrighted (C) 2001 by the Regents of the University of + * California; see the COPYRIGHT file for full details. Squid + * incorporates software developed and/or copyrighted by other + * sources; see the CREDITS file for full details. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111, USA. + * + */ + +#ifndef SQUID_ICAPINITIATE_H +#define SQUID_ICAPINITIATE_H + +#include "comm.h" +#include "MemBuf.h" +#include "ICAPServiceRep.h" +#include "AsyncCall.h" + +/* + * The ICAP Initiate is a common base for ICAP queries or transactions + * initiated by an ICAPInitiator. This interface exists to allow an ICAP + * initiator to signal its queries or transactions that it is aborting and + * no longer expecting an ICAP answer. The class is also handy for + * implementing common initiate actions such as maintaining and notifying + * the initiator. + * + * ICAPInitiate implementations must cbdata-protect themselves. + * + * Currently, only ICAPXaction is using this class, but this will change + * when we start supporting multiple ICAP transactions per ICAP query. + * + * This class could have been named ICAPInitiatee. + */ + +class HttpMsg; +class ICAPInitiator; + +class ICAPInitiate +{ + +public: + ICAPInitiate(ICAPInitiator *anInitiator, ICAPServiceRep::Pointer &aService); + virtual ~ICAPInitiate(); + + // start handler, treat as protected + virtual void start() = 0; + AsyncCallWrapper(93,3, ICAPInitiate, start); + + // communication with the initiator + virtual void noteInitiatorAborted() = 0; + AsyncCallWrapper(93,3, ICAPInitiate, noteInitiatorAborted) + +protected: + ICAPServiceRep &service(); + + void sendAnswer(HttpMsg *msg); // send to the initiator + void tellQueryAborted(); // tell initiator + void clearInitiator(); // used by noteInitiatorAborted; TODO: make private + + virtual void swanSong(); // internal cleanup + + virtual const char *status() const; // for debugging + + ICAPInitiator *theInitiator; + ICAPServiceRep::Pointer theService; +}; + +#endif /* SQUID_ICAPINITIATE_H */ Index: squid3/src/ICAP/ICAPInitiator.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPInitiator.cc,v retrieving revision 1.1.2.1 retrieving revision 1.1.2.2 diff -u -r1.1.2.1 -r1.1.2.2 --- squid3/src/ICAP/ICAPInitiator.cc 14 Feb 2007 05:43:12 -0000 1.1.2.1 +++ squid3/src/ICAP/ICAPInitiator.cc 23 Apr 2007 16:23:52 -0000 1.1.2.2 @@ -1 +1,26 @@ -// XXX: remove me! +/* + * DEBUG: section 93 ICAP (RFC 3507) Client + */ + +#include "squid.h" +#include "ICAPXaction.h" +#include "ICAPInitiator.h" + +ICAPXaction *ICAPInitiator::startIcapXaction(ICAPXaction *x) { + assert(x); + cbdataReference(x); + return ICAPXaction::AsyncStart(x); +} + +void ICAPInitiator::clearIcapXaction(ICAPXaction *&x) { + assert(x); + cbdataReferenceDone(x); +} + +void ICAPInitiator::announceInitiatorAbort(ICAPXaction *&x) +{ + if (x) { + AsyncCall(93,5, x, ICAPXaction::noteInitiatorAborted); + clearIcapXaction(x); + } +} Index: squid3/src/ICAP/ICAPInitiator.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPInitiator.h,v retrieving revision 1.1.2.1 retrieving revision 1.1.2.2 diff -u -r1.1.2.1 -r1.1.2.2 --- squid3/src/ICAP/ICAPInitiator.h 14 Feb 2007 05:43:13 -0000 1.1.2.1 +++ squid3/src/ICAP/ICAPInitiator.h 23 Apr 2007 16:23:53 -0000 1.1.2.2 @@ -1,6 +1,6 @@ /* - * $Id: ICAPInitiator.h,v 1.1.2.1 2007/02/14 05:43:13 rousskov Exp $ + * $Id: ICAPInitiator.h,v 1.1.2.2 2007/04/23 16:23:53 rousskov Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -39,9 +39,13 @@ /* * The ICAP Initiator is an ICAP vectoring point that initates ICAP * transactions. This interface exists to allow ICAP transactions to - * signal their initiators that they are finished or aborted. + * signal their initiators that they have the answer from the ICAP server + * or that the ICAP query has aborted and there will be no answer. It + * is also handy for implementing common initiator actions such as starting + * or aborting an ICAP transaction. */ +class HttpMsg; class ICAPXaction; class ICAPInitiator @@ -49,11 +53,21 @@ public: virtual ~ICAPInitiator() {} - virtual void noteIcapHeadersAdapted() = 0; - virtual void noteIcapHeadersAborted() = 0; + // called when ICAP response headers are successfully interpreted + virtual void noteIcapAnswer(HttpMsg *message) = 0; - AsyncCallWrapper(93,4, ICAPInitiator, noteIcapHeadersAdapted); - AsyncCallWrapper(93,3, ICAPInitiator, noteIcapHeadersAborted); + // called when valid ICAP response headers are no longer expected + virtual void noteIcapQueryAborted() = 0; + AsyncCallWrapper(93,3, ICAPInitiator, noteIcapQueryAborted); + +protected: + ICAPXaction *startIcapXaction(ICAPXaction *x); // locks and returns x + + // done with x (and not calling announceInitiatorAbort) + void clearIcapXaction(ICAPXaction *&x); // unlocks x + + // inform the transaction about abnormal termination and clear it + void announceInitiatorAbort(ICAPXaction *&x); // unlocks x }; #endif /* SQUID_ICAPINITIATOR_H */ Index: squid3/src/ICAP/ICAPModXact.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPModXact.cc,v retrieving revision 1.1.2.25 retrieving revision 1.1.2.26 diff -u -r1.1.2.25 -r1.1.2.26 --- squid3/src/ICAP/ICAPModXact.cc 21 Mar 2007 16:21:54 -0000 1.1.2.25 +++ squid3/src/ICAP/ICAPModXact.cc 23 Apr 2007 16:23:53 -0000 1.1.2.26 @@ -9,6 +9,7 @@ #include "HttpReply.h" #include "ICAPServiceRep.h" #include "ICAPInitiator.h" +#include "ICAPLauncher.h" #include "ICAPModXact.h" #include "ICAPClient.h" #include "ChunkedCodingParser.h" @@ -37,16 +38,13 @@ ICAPModXact::ICAPModXact(ICAPInitiator *anInitiator, HttpMsg *virginHeader, HttpRequest *virginCause, ICAPServiceRep::Pointer &aService): - ICAPXaction("ICAPModXact"), - initiator(cbdataReference(anInitiator)), + ICAPXaction("ICAPModXact", anInitiator, aService), icapReply(NULL), virginConsumed(0), bodyParser(NULL) { assert(virginHeader); - service(aService); - virgin.setHeader(virginHeader); // sets virgin.body_pipe if needed virgin.setCause(virginCause); // may be NULL @@ -581,8 +579,7 @@ return; } - AsyncCall(93,5, initiator, ICAPInitiator::noteIcapHeadersAdapted); - cbdataReferenceDone(initiator); + sendAnswer(adapted.header); if (state.sending == State::sendingVirgin) echoMore(); @@ -920,19 +917,6 @@ ICAPXaction_Exit(); } -// initiator aborted -void ICAPModXact::noteInitiatorAborted() -{ - ICAPXaction_Enter(noteInitiatorAborted); - - if (initiator) { - cbdataReferenceDone(initiator); - mustStop("initiator aborted"); - } - - ICAPXaction_Exit(); -} - // adapted body consumer wants more adapted data and // possibly freed some buffer space void ICAPModXact::noteMoreBodySpaceAvailable(BodyPipe &) @@ -964,12 +948,6 @@ { debugs(93, 5, HERE << "swan sings" << status()); - if (initiator) { -debugs(93, 2, HERE << "swan sings for " << stopReason << status()); - AsyncCall(93,5, initiator, ICAPInitiator::noteIcapHeadersAborted); - cbdataReferenceDone(initiator); - } - stopWriting(false); stopBackup(); Index: squid3/src/ICAP/ICAPModXact.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPModXact.h,v retrieving revision 1.1.2.11 retrieving revision 1.1.2.12 diff -u -r1.1.2.11 -r1.1.2.12 --- squid3/src/ICAP/ICAPModXact.h 21 Mar 2007 16:21:55 -0000 1.1.2.11 +++ squid3/src/ICAP/ICAPModXact.h 23 Apr 2007 16:23:53 -0000 1.1.2.12 @@ -1,6 +1,6 @@ /* - * $Id: ICAPModXact.h,v 1.1.2.11 2007/03/21 16:21:55 rousskov Exp $ + * $Id: ICAPModXact.h,v 1.1.2.12 2007/04/23 16:23:53 rousskov Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -132,10 +132,6 @@ public: ICAPModXact(ICAPInitiator *anInitiator, HttpMsg *virginHeader, HttpRequest *virginCause, ICAPServiceRep::Pointer &s); - // communication with the initiator - void noteInitiatorAborted(); - AsyncCallWrapper(93,3, ICAPModXact, noteInitiatorAborted) - // BodyProducer methods virtual void noteMoreBodySpaceAvailable(BodyPipe &); virtual void noteBodyConsumerAborted(BodyPipe &); @@ -237,7 +233,6 @@ bool gotEncapsulated(const char *section) const; void checkConsuming(); - ICAPInitiator *initiator; HttpReply *icapReply; Index: squid3/src/ICAP/ICAPOptXact.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPOptXact.cc,v retrieving revision 1.1.2.7 retrieving revision 1.1.2.8 diff -u -r1.1.2.7 -r1.1.2.8 --- squid3/src/ICAP/ICAPOptXact.cc 14 Feb 2007 06:40:45 -0000 1.1.2.7 +++ squid3/src/ICAP/ICAPOptXact.cc 23 Apr 2007 16:23:54 -0000 1.1.2.8 @@ -12,20 +12,9 @@ CBDATA_CLASS_INIT(ICAPOptXact); -ICAPOptXact::ICAPOptXact(ICAPServiceRep::Pointer &aService, Callback *aCbAddr, void *aCbData): - ICAPXaction("ICAPOptXact"), - cbAddr(aCbAddr), cbData(cbdataReference(aCbData)) +ICAPOptXact::ICAPOptXact(ICAPInitiator *anInitiator, ICAPServiceRep::Pointer &aService): + ICAPXaction("ICAPOptXact", anInitiator, aService) { - Must(aCbAddr && aCbData); - service(aService); -} - -ICAPOptXact::~ICAPOptXact() -{ - if (cbAddr) { - debugs(93, 1, HERE << "BUG: exiting without sending options"); - cbdataReferenceDone(cbData); - } } void ICAPOptXact::start() @@ -34,7 +23,7 @@ ICAPXaction::start(); - Must(self != NULL); // set by AsyncStart; + Must(cbdataReferenceValid(this)); // set by AsyncStart openConnection(); @@ -71,8 +60,8 @@ // comm module read a portion of the ICAP response for us void ICAPOptXact::handleCommRead(size_t) { - if (ICAPOptions *options = parseResponse()) { - sendOptions(options); + if (HttpMsg *r = parseResponse()) { + sendAnswer(r); Must(done()); // there should be nothing else to do return; } @@ -80,7 +69,7 @@ scheduleRead(); } -ICAPOptions *ICAPOptXact::parseResponse() +HttpMsg *ICAPOptXact::parseResponse() { debugs(93, 5, HERE << "have " << readBuf.contentSize() << " bytes to parse" << status()); @@ -97,34 +86,5 @@ if (httpHeaderHasConnDir(&r->header, "close")) reuseConnection = false; - ICAPOptions *options = new ICAPOptions; - options->configure(r); - - delete r; - - return options; -} - -void ICAPOptXact::swanSong() { - if (cbAddr) { - debugs(93, 3, HERE << "probably failed; sending NULL options"); - sendOptions(0); - } - ICAPXaction::swanSong(); -} - -void ICAPOptXact::sendOptions(ICAPOptions *options) { - debugs(93, 3, HERE << "sending options " << options << " to " << cbData << - " at " << (void*)cbAddr << status()); - - Must(cbAddr); - Callback *addr = cbAddr; - cbAddr = NULL; // in case the callback calls us or throws - - void *data = NULL; - if (cbdataReferenceValidDone(cbData, &data)) - (*addr)(options, data); // callee takes ownership of the options - else - debugs(93, 2, HERE << "sending options " << options << " to " << - data << " failed" << status()); + return r; } Index: squid3/src/ICAP/ICAPOptXact.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPOptXact.h,v retrieving revision 1.1.2.4 retrieving revision 1.1.2.5 diff -u -r1.1.2.4 -r1.1.2.5 --- squid3/src/ICAP/ICAPOptXact.h 14 Feb 2007 06:40:45 -0000 1.1.2.4 +++ squid3/src/ICAP/ICAPOptXact.h 23 Apr 2007 16:23:54 -0000 1.1.2.5 @@ -1,5 +1,5 @@ /* - * $Id: ICAPOptXact.h,v 1.1.2.4 2007/02/14 06:40:45 rousskov Exp $ + * $Id: ICAPOptXact.h,v 1.1.2.5 2007/04/23 16:23:54 rousskov Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -36,41 +36,32 @@ #include "ICAPXaction.h" class ICAPOptions; +class HttpMsg; /* ICAPOptXact sends an ICAP OPTIONS request to the ICAP service, - * converts the response into ICAPOptions object, and notifies - * the caller via the callback. NULL options objects means the - * ICAP service could not be contacted or did not return any response */ + * parses the ICAP response, and sends it to the initiator. A NULL response + * means the ICAP service could not be contacted or did not return any + * valid response. */ class ICAPOptXact: public ICAPXaction { public: - typedef void Callback(ICAPOptions *newOptions, void *callerData); - - ICAPOptXact(ICAPServiceRep::Pointer &aService, Callback *aCb, void *aCbData); - virtual ~ICAPOptXact(); + ICAPOptXact(ICAPInitiator *anInitiator, ICAPServiceRep::Pointer &aService); protected: virtual void start(); virtual void handleCommConnected(); virtual void handleCommWrote(size_t size); virtual void handleCommRead(size_t size); - virtual void swanSong(); void makeRequest(MemBuf &buf); - ICAPOptions *parseResponse(); - void sendOptions(ICAPOptions *options); + HttpMsg *parseResponse(); void startReading(); private: - Callback *cbAddr; // callback to call with newly fetched options - void *cbData; // callback data - CBDATA_CLASS2(ICAPOptXact); }; -// TODO: replace the callback API with a class-base interface? - #endif /* SQUID_ICAPOPTXACT_H */ Index: squid3/src/ICAP/ICAPServiceRep.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPServiceRep.cc,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/ICAPServiceRep.cc 14 Feb 2007 06:49:37 -0000 1.1.2.13 +++ squid3/src/ICAP/ICAPServiceRep.cc 23 Apr 2007 16:23:54 -0000 1.1.2.14 @@ -4,6 +4,7 @@ #include "squid.h" #include "TextException.h" +#include "HttpReply.h" #include "ICAPServiceRep.h" #include "ICAPOptions.h" #include "ICAPOptXact.h" @@ -410,21 +411,38 @@ wasAnnouncedUp = !wasAnnouncedUp; } -static -void ICAPServiceRep_noteNewOptions(ICAPOptions *newOptions, void *data) +// we are receiving ICAP OPTIONS response headers here or NULL on failures +void ICAPServiceRep::noteIcapAnswer(HttpMsg *msg) { - ICAPServiceRep *service = static_cast(data); - Must(service); - service->noteNewOptions(newOptions); -} + Must(waiting); + waiting = false; -void ICAPServiceRep::noteNewOptions(ICAPOptions *newOptions) -{ - // newOptions may be NULL + Must(msg); + + debugs(93,5, "ICAPService is interpreting new options " << status()); + ICAPOptions *newOptions = NULL; + if (HttpReply *r = dynamic_cast(msg)) { + newOptions = new ICAPOptions; + newOptions->configure(r); + } else { + debugs(93,1, "ICAPService got wrong options message " << status()); + } + + handleNewOptions(newOptions); +} + +void ICAPServiceRep::noteIcapQueryAborted() { Must(waiting); waiting = false; + debugs(93,3, "ICAPService failed to fetch options " << status()); + handleNewOptions(0); +} + +void ICAPServiceRep::handleNewOptions(ICAPOptions *newOptions) +{ + // new options may be NULL changeOptions(newOptions); debugs(93,3, "ICAPService got new options and is now " << status()); @@ -439,9 +457,9 @@ debugs(93,6, "ICAPService will get new options " << status()); waiting = true; - ICAPOptXact::AsyncStart( - new ICAPOptXact(self, &ICAPServiceRep_noteNewOptions, this)); + startIcapXaction(new ICAPOptXact(this, self)); // TODO: timeout in case ICAPOptXact never calls us back? + // Such a timeout should probably be a generic AsyncStart feature. } void ICAPServiceRep::scheduleUpdate() Index: squid3/src/ICAP/ICAPServiceRep.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPServiceRep.h,v retrieving revision 1.1.2.9 retrieving revision 1.1.2.10 diff -u -r1.1.2.9 -r1.1.2.10 --- squid3/src/ICAP/ICAPServiceRep.h 14 Feb 2007 06:49:37 -0000 1.1.2.9 +++ squid3/src/ICAP/ICAPServiceRep.h 23 Apr 2007 16:23:54 -0000 1.1.2.10 @@ -1,6 +1,6 @@ /* - * $Id: ICAPServiceRep.h,v 1.1.2.9 2007/02/14 06:49:37 rousskov Exp $ + * $Id: ICAPServiceRep.h,v 1.1.2.10 2007/04/23 16:23:54 rousskov Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -35,10 +35,10 @@ #define SQUID_ICAPSERVICEREP_H #include "cbdata.h" +#include "ICAPInitiator.h" #include "ICAPElements.h" class ICAPOptions; - class ICAPOptXact; /* The ICAP service representative maintains information about a single ICAP @@ -67,7 +67,7 @@ */ -class ICAPServiceRep : public RefCountable +class ICAPServiceRep : public RefCountable, public ICAPInitiator { public: @@ -114,7 +114,10 @@ public: // treat these as private, they are for callbacks only void noteTimeToUpdate(); void noteTimeToNotify(); - void noteNewOptions(ICAPOptions *newOptions); + + // receive either an ICAP OPTIONS response header or an abort message + virtual void noteIcapAnswer(HttpMsg *msg); + virtual void noteIcapQueryAborted(); private: // stores Prepare() callback info @@ -153,6 +156,7 @@ void scheduleNotification(); void startGettingOptions(); + void handleNewOptions(ICAPOptions *newOptions); void changeOptions(ICAPOptions *newOptions); void checkOptions(); Index: squid3/src/ICAP/ICAPXaction.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPXaction.cc,v retrieving revision 1.1.2.12 retrieving revision 1.1.2.13 diff -u -r1.1.2.12 -r1.1.2.13 --- squid3/src/ICAP/ICAPXaction.cc 16 Apr 2007 19:49:22 -0000 1.1.2.12 +++ squid3/src/ICAP/ICAPXaction.cc 23 Apr 2007 16:23:55 -0000 1.1.2.13 @@ -61,12 +61,13 @@ ICAPXaction *ICAPXaction::AsyncStart(ICAPXaction *x) { assert(x != NULL); - x->self = x; // yes, this works with the current RefCount cimplementation + cbdataReference(x); // unlocked when done() in callEnd() AsyncCall(93,5, x, ICAPXaction::start); return x; } -ICAPXaction::ICAPXaction(const char *aTypeName): +ICAPXaction::ICAPXaction(const char *aTypeName, ICAPInitiator *anInitiator, ICAPServiceRep::Pointer &aService): + ICAPInitiate(anInitiator, aService), id(++TheLastId), connection(-1), commBuf(NULL), commBufSize(0), @@ -74,7 +75,6 @@ reuseConnection(true), connector(NULL), reader(NULL), writer(NULL), closer(NULL), typeName(aTypeName), - theService(NULL), inCall(NULL) { debugs(93,3, typeName << " constructed, this=" << this << @@ -91,7 +91,7 @@ { debugs(93,3, HERE << typeName << " starts" << status()); - Must(self != NULL); // set by AsyncStart; + Must(cbdataReferenceValid(this)); // locked by AsyncStart readBuf.init(SQUID_TCP_SO_RCVBUF, SQUID_TCP_SO_RCVBUF); commBuf = (char*)memAllocBuf(SQUID_TCP_SO_RCVBUF, &commBufSize); @@ -399,6 +399,19 @@ doneReading() && doneWriting(); } +// initiator aborted +void ICAPXaction::noteInitiatorAborted() +{ + ICAPXaction_Enter(noteInitiatorAborted); + + if (theInitiator) { + clearInitiator(); + mustStop("initiator aborted"); + } + + ICAPXaction_Exit(); +} + void ICAPXaction::mustStop(const char *aReason) { Must(inCall); // otherwise nobody will delete us if we are done() @@ -426,20 +439,7 @@ if (commBuf) memFreeBuf(commBufSize, commBuf); - debugs(93, 5, HERE << "swan sang" << status()); -} - -void ICAPXaction::service(ICAPServiceRep::Pointer &aService) -{ - Must(!theService); - Must(aService != NULL); - theService = aService; -} - -ICAPServiceRep &ICAPXaction::service() -{ - Must(theService != NULL); - return *theService; + ICAPInitiate::swanSong(); } bool ICAPXaction::callStart(const char *method) @@ -455,13 +455,6 @@ return false; } - if (!self) { - // this may happen when swanSong() has not properly cleaned up. - debugs(93, 5, HERE << typeName << "::" << method << - " is not admitted to a finished transaction " << this); - return false; - } - inCall = method; return true; } @@ -483,10 +476,15 @@ swanSong(); const char *inCallSaved = inCall; const char *typeNameSaved = typeName; + void *object = this; inCall = NULL; - self = NULL; // will delete us, now or eventually + + delete this; + cbdataReferenceDone(object); // locked by AsyncStart + + // careful: this object does not exist any more debugs(93, 6, HERE << typeNameSaved << "::" << inCallSaved << - " ended " << this); + " ended " << object); return; } else if (doneWithIo()) { Index: squid3/src/ICAP/ICAPXaction.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPXaction.h,v retrieving revision 1.1.2.7 retrieving revision 1.1.2.8 diff -u -r1.1.2.7 -r1.1.2.8 --- squid3/src/ICAP/ICAPXaction.h 14 Feb 2007 07:00:45 -0000 1.1.2.7 +++ squid3/src/ICAP/ICAPXaction.h 23 Apr 2007 16:23:55 -0000 1.1.2.8 @@ -1,6 +1,6 @@ /* - * $Id: ICAPXaction.h,v 1.1.2.7 2007/02/14 07:00:45 rousskov Exp $ + * $Id: ICAPXaction.h,v 1.1.2.8 2007/04/23 16:23:55 rousskov Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -37,6 +37,7 @@ #include "comm.h" #include "MemBuf.h" #include "ICAPServiceRep.h" +#include "ICAPInitiate.h" #include "AsyncCall.h" class HttpMsg; @@ -44,29 +45,24 @@ /* * The ICAP Xaction implements common tasks for ICAP OPTIONS, REQMOD, and - * RESPMOD transactions. - * - * All ICAP transactions are refcounted and hold a pointer to self. - * Both are necessary because a user need to access transaction data - * after the transaction has finished, while a transaction may need to - * finish after all its explicit users are gone. For safety and simplicity, - * the code assumes that both cases can happen to any ICAP transaction. + * RESPMOD transactions. It is started by an ICAPInitiator. It terminates + * on its own, when done. Transactions communicate with Initiator using + * asynchronous messages because a transaction or Initiator may be gone at + * any time. */ // Note: ICAPXaction must be the first parent for object-unaware cbdata to work -class ICAPXaction: public RefCountable +class ICAPXaction: public ICAPInitiate { public: - typedef RefCount Pointer; - - // Use this to start ICAP transactions because they need a pointer - // to self and because the start routine may result in failures/callbacks. + // Use this to start ICAP transactions because + // the start routine may result in failures/callbacks. static ICAPXaction *AsyncStart(ICAPXaction *x); public: - ICAPXaction(const char *aTypeName); + ICAPXaction(const char *aTypeName, ICAPInitiator *anInitiator, ICAPServiceRep::Pointer &aService); virtual ~ICAPXaction(); // comm handler wrappers, treat as private @@ -76,14 +72,10 @@ void noteCommTimedout(); void noteCommClosed(); - // start handler, treat as protected and call it from the kids - virtual void start() = 0; - AsyncCallWrapper(93,3, ICAPXaction, start); - protected: - // Set or get service pointer; ICAPXaction cbdata-locks it. - void service(ICAPServiceRep::Pointer &aService); - ICAPServiceRep &service(); + virtual void start() = 0; // has body + + virtual void noteInitiatorAborted(); // TODO: move to ICAPInitiate // comm hanndlers; called by comm handler wrappers virtual void handleCommConnected() = 0; @@ -124,7 +116,6 @@ virtual bool fillVirginHttpHeader(MemBuf&) const; protected: - Pointer self; // see comments in the class description above const int id; // transaction ID for debugging, unique across ICAP xactions int connection; // FD of the ICAP server connection @@ -163,7 +154,6 @@ private: static int TheLastId; - ICAPServiceRep::Pointer theService; const char *inCall; // name of the asynchronous call being executed, if any