--------------------- PatchSet 4048 Date: 2007/02/14 06:40:45 Author: rousskov Branch: squid3-icap Tag: (none) Log: - Use asynchronous start and swan song methods that are now supported by ICAPXaction. This should make transaction initiation and completion more robust in the presense of transaction errors and exceptions. - Do not store options, just send them to the initiator. This makes code simpler. - Destructor must cleanup callback cbdata if it is left over. Members: src/ICAP/ICAPOptXact.cc:1.1.2.6->1.1.2.7 src/ICAP/ICAPOptXact.h:1.1.2.3->1.1.2.4 Index: squid3/src/ICAP/ICAPOptXact.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPOptXact.cc,v retrieving revision 1.1.2.6 retrieving revision 1.1.2.7 diff -u -r1.1.2.6 -r1.1.2.7 --- squid3/src/ICAP/ICAPOptXact.cc 25 Oct 2006 04:21:33 -0000 1.1.2.6 +++ squid3/src/ICAP/ICAPOptXact.cc 14 Feb 2007 06:40:45 -0000 1.1.2.7 @@ -12,25 +12,29 @@ CBDATA_CLASS_INIT(ICAPOptXact); -ICAPOptXact::ICAPOptXact(): ICAPXaction("ICAPOptXact"), options(NULL), - cb(NULL), cbData(NULL) - +ICAPOptXact::ICAPOptXact(ICAPServiceRep::Pointer &aService, Callback *aCbAddr, void *aCbData): + ICAPXaction("ICAPOptXact"), + cbAddr(aCbAddr), cbData(cbdataReference(aCbData)) { + Must(aCbAddr && aCbData); + service(aService); } ICAPOptXact::~ICAPOptXact() { - Must(!options); // the caller must set to NULL + if (cbAddr) { + debugs(93, 1, HERE << "BUG: exiting without sending options"); + cbdataReferenceDone(cbData); + } } -void ICAPOptXact::start(ICAPServiceRep::Pointer &aService, Callback *aCb, void *aCbData) +void ICAPOptXact::start() { ICAPXaction_Enter(start); - service(aService); - Must(!cb && aCb && aCbData); - cb = aCb; - cbData = cbdataReference(aCbData); + ICAPXaction::start(); + + Must(self != NULL); // set by AsyncStart; openConnection(); @@ -50,32 +54,6 @@ scheduleWrite(requestBuf); } -bool ICAPOptXact::doneAll() const -{ - return options && ICAPXaction::doneAll(); -} - - -void ICAPOptXact::doStop() -{ - ICAPXaction::doStop(); - - if (Callback *call = cb) { - cb = NULL; - void *data = NULL; - - if (cbdataReferenceValidDone(cbData, &data)) { - (*call)(this, data); // will delete us - return; - } - } - - // get rid of options if we did not call the callback - delete options; - - options = NULL; -} - void ICAPOptXact::makeRequest(MemBuf &buf) { const ICAPServiceRep &s = service(); @@ -93,13 +71,16 @@ // comm module read a portion of the ICAP response for us void ICAPOptXact::handleCommRead(size_t) { - if (parseResponse()) + if (ICAPOptions *options = parseResponse()) { + sendOptions(options); Must(done()); // there should be nothing else to do - else - scheduleRead(); + return; + } + + scheduleRead(); } -bool ICAPOptXact::parseResponse() +ICAPOptions *ICAPOptXact::parseResponse() { debugs(93, 5, HERE << "have " << readBuf.contentSize() << " bytes to parse" << status()); @@ -108,19 +89,42 @@ HttpReply *r = new HttpReply; r->protoPrefix = "ICAP/"; // TODO: make an IcapReply class? - if (!parseHttpMsg(r)) { + if (!parseHttpMsg(r)) { // throws on errors delete r; - return false; + return 0; } - options = new ICAPOptions; - - options->configure(r); - if (httpHeaderHasConnDir(&r->header, "close")) reuseConnection = false; + ICAPOptions *options = new ICAPOptions; + options->configure(r); + delete r; - return true; + 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()); } Index: squid3/src/ICAP/ICAPOptXact.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ICAP/ICAPOptXact.h,v retrieving revision 1.1.2.3 retrieving revision 1.1.2.4 diff -u -r1.1.2.3 -r1.1.2.4 --- squid3/src/ICAP/ICAPOptXact.h 6 Oct 2006 16:51:36 -0000 1.1.2.3 +++ squid3/src/ICAP/ICAPOptXact.h 14 Feb 2007 06:40:45 -0000 1.1.2.4 @@ -1,5 +1,5 @@ /* - * $Id: ICAPOptXact.h,v 1.1.2.3 2006/10/06 16:51:36 rousskov Exp $ + * $Id: ICAPOptXact.h,v 1.1.2.4 2007/02/14 06:40:45 rousskov Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -46,33 +46,31 @@ { public: - typedef void Callback(ICAPOptXact*, void *data); + typedef void Callback(ICAPOptions *newOptions, void *callerData); - ICAPOptXact(); + ICAPOptXact(ICAPServiceRep::Pointer &aService, Callback *aCb, void *aCbData); virtual ~ICAPOptXact(); - void start(ICAPServiceRep::Pointer &aService, Callback *aCb, void *aCbData); - - ICAPOptions *options; // result for the caller to take/handle - protected: + virtual void start(); virtual void handleCommConnected(); virtual void handleCommWrote(size_t size); virtual void handleCommRead(size_t size); - virtual bool doneAll() const; + virtual void swanSong(); void makeRequest(MemBuf &buf); - bool parseResponse(); + ICAPOptions *parseResponse(); + void sendOptions(ICAPOptions *options); void startReading(); - virtual void doStop(); - private: - Callback *cb; - void *cbData; + 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 */