From c6f949a06515fc0c66e9bb724d24c3fd3b0e0134 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Fri, 28 May 2010 00:10:57 -0600 Subject: [PATCH] Author: Alex Rousskov Bug 2697: Adaptation leaks and extra requests after reconfiguration This patch "detaches" services from the configuration during reconfiguration. Detached services do not participate in new adaptation transactions but allow the old transactions to finish nicely. Once all users are gone, the refcounted service disappears. As a side effect of these fixes, several aspects of eCAP service registration and mapping of loaded and configured eCAP services have been fixed. We will be able to claim support for eCAP reconfiguration after libecap adds reconfiguration API. --- src/adaptation/Config.cc | 14 ++-- src/adaptation/Service.cc | 6 ++ src/adaptation/Service.h | 12 +++- src/adaptation/ecap/Config.cc | 5 +- src/adaptation/ecap/Host.cc | 39 +++++------ src/adaptation/ecap/Host.h | 9 ++- src/adaptation/ecap/ServiceRep.cc | 113 +++++++++++++++++++++++++----- src/adaptation/ecap/ServiceRep.h | 18 +++-- src/adaptation/icap/Config.cc | 4 +- src/adaptation/icap/ServiceRep.cc | 55 +++++++-------- src/adaptation/icap/ServiceRep.h | 12 ++-- src/main.cc | 18 +++++ 12 files changed, 204 insertions(+), 101 deletions(-) diff --git a/src/adaptation/Config.cc b/src/adaptation/Config.cc index e7f9332e8c..2bdce463f1 100644 --- a/src/adaptation/Config.cc +++ b/src/adaptation/Config.cc @@ -62,6 +62,11 @@ Adaptation::Config::parseService() void Adaptation::Config::freeService() { + FreeAccess(); + FreeServiceGroups(); + + DetachServices(); + while (!serviceConfigs.empty()) { delete serviceConfigs.back(); serviceConfigs.pop_back(); @@ -208,14 +213,5 @@ Adaptation::Config::Config() // with global arrays shared by those individual configs Adaptation::Config::~Config() { - FreeAccess(); - FreeServiceGroups(); - - // invalidate each service so that it can be deleted when refcount=0 - while (!AllServices().empty()) { - AllServices().back()->invalidate(); - AllServices().pop_back(); - } - freeService(); } diff --git a/src/adaptation/Service.cc b/src/adaptation/Service.cc index 4ce90e8e2b..7b9729a221 100644 --- a/src/adaptation/Service.cc +++ b/src/adaptation/Service.cc @@ -68,3 +68,9 @@ Adaptation::FindService(const Service::Id& key) } return NULL; } + +void Adaptation::DetachServices() +{ + while (!AllServices().empty()) + AllServices().pop_back()->detach(); +} diff --git a/src/adaptation/Service.h b/src/adaptation/Service.h index 99a15bb1f1..ab770d9fcd 100644 --- a/src/adaptation/Service.h +++ b/src/adaptation/Service.h @@ -27,9 +27,6 @@ public: Service(const ServiceConfig &aConfig); virtual ~Service(); - // call when the service is no longer needed or valid - virtual void invalidate() = 0; - virtual bool probed() const = 0; // see comments above virtual bool broken() const; virtual bool up() const = 0; // see comments above @@ -51,6 +48,12 @@ public: virtual void finalize(); // called after creation + /// called when removed from the config; the service will be + /// auto-destroyed when the last refcounting user leaves + virtual void detach() = 0; + /// whether detached() was called + virtual bool detached() const = 0; + protected: ServiceConfig &writeableCfg() { return theConfig; } @@ -64,6 +67,9 @@ typedef Vector Services; extern Services &AllServices(); extern ServicePointer FindService(const Service::Id &key); +/// detach all adaptation services from current configuration +extern void DetachServices(); + } // namespace Adaptation #endif /* SQUID_ADAPTATION__SERVICE_H */ diff --git a/src/adaptation/ecap/Config.cc b/src/adaptation/ecap/Config.cc index 70222f951b..c6ed5df8ca 100644 --- a/src/adaptation/ecap/Config.cc +++ b/src/adaptation/ecap/Config.cc @@ -4,7 +4,6 @@ */ #include "squid.h" -#include #include "adaptation/ecap/Host.h" #include "adaptation/ecap/ServiceRep.h" #include "adaptation/ecap/Config.h" @@ -23,8 +22,8 @@ void Adaptation::Ecap::Config::finalize() { Adaptation::Config::finalize(); - libecap::shared_ptr host(new Adaptation::Ecap::Host); - libecap::RegisterHost(host); + Host::Register(); + CheckUnusedAdapterServices(AllServices()); } Adaptation::ServicePointer diff --git a/src/adaptation/ecap/Host.cc b/src/adaptation/ecap/Host.cc index 0c24801eff..b98902d001 100644 --- a/src/adaptation/ecap/Host.cc +++ b/src/adaptation/ecap/Host.cc @@ -4,6 +4,7 @@ #include "squid.h" #include #include +#include #include "TextException.h" #include "adaptation/ecap/ServiceRep.h" #include "adaptation/ecap/Host.h" @@ -15,9 +16,14 @@ const libecap::Name Adaptation::Ecap::protocolIcp("ICP", libecap::Name::NextId() const libecap::Name Adaptation::Ecap::protocolHtcp("Htcp", libecap::Name::NextId()); #endif +/// the host application (i.e., Squid) wrapper registered with libecap +static libecap::shared_ptr TheHost; + Adaptation::Ecap::Host::Host() { // assign our host-specific IDs to well-known names + // this code can run only once + libecap::headerReferer.assignHostId(HDR_REFERER); libecap::protocolHttp.assignHostId(PROTO_HTTP); @@ -50,28 +56,8 @@ Adaptation::Ecap::Host::describe(std::ostream &os) const void Adaptation::Ecap::Host::noteService(const libecap::weak_ptr &weak) { - // Many ecap_service lines may use the same service URI. Find each - // matching service rep, make sure it is an eCAP rep, - // and update it with the actual eCAP service. - int found = 0; - - libecap::shared_ptr shared(weak); - typedef Adaptation::Services::iterator SI; - for (SI i = Adaptation::AllServices().begin(); i != Adaptation::AllServices().end(); ++i) { - if ((*i)->cfg().uri == shared->uri().c_str()) { - ServiceRep *rep = dynamic_cast(i->getRaw()); - Must(rep); - rep->noteService(shared); - ++found; - } - } - - debugs(93,5, HERE << "Found " << found << " ecap_service configs for " << - shared->uri()); - if (!found) { - debugs(93,1, "Warning: ignoring loaded eCAP module service without " << - "a matching ecap_service configuration: " << shared->uri()); - } + Must(!weak.expired()); + RegisterAdapterService(weak.lock()); } static int @@ -107,3 +93,12 @@ Adaptation::Ecap::Host::closeDebug(std::ostream *debug) if (debug) Debug::finishDebug(); } + +void +Adaptation::Ecap::Host::Register() +{ + if (!TheHost) { + TheHost.reset(new Adaptation::Ecap::Host); + libecap::RegisterHost(TheHost); + } +} diff --git a/src/adaptation/ecap/Host.h b/src/adaptation/ecap/Host.h index 85f257bf55..8d5f32fa02 100644 --- a/src/adaptation/ecap/Host.h +++ b/src/adaptation/ecap/Host.h @@ -17,8 +17,6 @@ namespace Ecap class Host : public libecap::host::Host { public: - Host(); - // About virtual std::string uri() const; // unique across all vendors virtual void describe(std::ostream &os) const; // free-format info @@ -29,6 +27,13 @@ public: // Logging virtual std::ostream *openDebug(libecap::LogVerbosity lv); virtual void closeDebug(std::ostream *debug); + + static void Register(); ///< register adaptation host + +private: + Host(); + Host (const Host&); ///< not implemented + Host& operator= (const Host&); ///< not implemented }; extern const libecap::Name protocolInternal; diff --git a/src/adaptation/ecap/ServiceRep.cc b/src/adaptation/ecap/ServiceRep.cc index e4188306c6..0961cb8410 100644 --- a/src/adaptation/ecap/ServiceRep.cc +++ b/src/adaptation/ecap/ServiceRep.cc @@ -2,13 +2,18 @@ * DEBUG: section 93 eCAP Interface */ #include "squid.h" +#include #include #include "TextException.h" #include "adaptation/ecap/ServiceRep.h" #include "adaptation/ecap/XactionRep.h" +// configured eCAP service wrappers +static std::list TheServices; + Adaptation::Ecap::ServiceRep::ServiceRep(const Adaptation::ServiceConfig &cfg): - /*AsyncJob("Adaptation::Ecap::ServiceRep"),*/ Adaptation::Service(cfg) + /*AsyncJob("Adaptation::Ecap::ServiceRep"),*/ Adaptation::Service(cfg), + isDetached(false) { } @@ -16,20 +21,6 @@ Adaptation::Ecap::ServiceRep::~ServiceRep() { } -void Adaptation::Ecap::ServiceRep::noteService(const AdapterService &s) -{ - Must(s != NULL); - theService = s; - debugs(93,7, HERE << "matched loaded and configured eCAP services: " << - s->uri() << ' ' << cfg().key << "\n"); -} - -void Adaptation::Ecap::ServiceRep::invalidate() -{ - theService->retire(); - theService.reset(); -} - void Adaptation::Ecap::ServiceRep::noteFailure() { assert(false); // XXX: should this be ICAP-specific? @@ -39,6 +30,7 @@ void Adaptation::Ecap::ServiceRep::finalize() { Adaptation::Service::finalize(); + theService = FindAdapterService(cfg().uri); if (theService) { debugs(93,3, HERE << "starting eCAP service: " << theService->uri()); theService->start(); @@ -78,7 +70,92 @@ Adaptation::Ecap::ServiceRep::makeXactLauncher(Adaptation::Initiator *initiator, // returns a temporary string depicting service status, for debugging const char *Adaptation::Ecap::ServiceRep::status() const { - assert(false); // move generic stuff from ICAP to Adaptation - // add theService->status()? - return NULL; + // TODO: move generic stuff from eCAP and ICAP to Adaptation + static MemBuf buf; + + buf.reset(); + buf.append("[", 1); + + if (up()) + buf.append("up", 2); + else + buf.append("down", 4); + + if (detached()) + buf.append(",detached", 9); + + buf.append("]", 1); + buf.terminate(); + + return buf.content(); +} + +void Adaptation::Ecap::ServiceRep::detach() +{ + isDetached = true; +} + +bool Adaptation::Ecap::ServiceRep::detached() const +{ + return isDetached; +} + +Adaptation::Ecap::ServiceRep::AdapterService +Adaptation::Ecap::FindAdapterService(const String& serviceUri) +{ + typedef std::list::const_iterator ASCI; + for (ASCI s = TheServices.begin(); s != TheServices.end(); ++s) { + Must(*s); + if (serviceUri == (*s)->uri().c_str()) + return *s; + } + return ServiceRep::AdapterService(); +} + +void +Adaptation::Ecap::RegisterAdapterService(const Adaptation::Ecap::ServiceRep::AdapterService& adapterService) +{ + typedef std::list::iterator ASI; + for (ASI s = TheServices.begin(); s != TheServices.end(); ++s) { + Must(*s); + if (adapterService->uri() == (*s)->uri()) { + *s = adapterService; + debugs(93, 3, "updated eCAP module service: " << + adapterService->uri()); + return; + } + } + TheServices.push_back(adapterService); + debugs(93, 3, "registered eCAP module service: " << adapterService->uri()); +} + +void +Adaptation::Ecap::UnregisterAdapterService(const String& serviceUri) +{ + typedef std::list::iterator ASI; + for (ASI s = TheServices.begin(); s != TheServices.end(); ++s) { + if (serviceUri == (*s)->uri().c_str()) { + TheServices.erase(s); + debugs(93, 3, "unregistered eCAP module service: " << serviceUri); + return; + } + } + debugs(93, 3, "failed to unregister eCAP module service: " << serviceUri); +} + +void +Adaptation::Ecap::CheckUnusedAdapterServices(const Adaptation::Services& cfgs) +{ + typedef std::list::const_iterator ASCI; + for (ASCI loaded = TheServices.begin(); loaded != TheServices.end(); + ++loaded) { + bool found = false; + for (Services::const_iterator cfged = cfgs.begin(); + cfged != cfgs.end() && !found; ++cfged) { + found = (*cfged)->cfg().uri == (*loaded)->uri().c_str(); + } + if (!found) + debugs(93, 1, "Warning: loaded eCAP service has no matching " << + "ecap_service config option: " << (*loaded)->uri()); + } } diff --git a/src/adaptation/ecap/ServiceRep.h b/src/adaptation/ecap/ServiceRep.h index ed4c7d376c..27046b9a98 100644 --- a/src/adaptation/ecap/ServiceRep.h +++ b/src/adaptation/ecap/ServiceRep.h @@ -27,13 +27,9 @@ public: virtual ~ServiceRep(); typedef libecap::shared_ptr AdapterService; - void noteService(const AdapterService &s); virtual void finalize(); - // call when the service is no longer needed or valid - virtual void invalidate(); - virtual bool probed() const; virtual bool up() const; @@ -47,10 +43,24 @@ public: virtual const char *status() const; + virtual void detach(); + virtual bool detached() const; + private: AdapterService theService; // the actual adaptation service we represent + bool isDetached; }; +/// register loaded eCAP module service +extern void RegisterAdapterService(const ServiceRep::AdapterService& adapterService); +/// unregister loaded eCAP module service by service uri +extern void UnregisterAdapterService(const String& serviceUri); + +/// returns loaded eCAP module service by service uri +extern ServiceRep::AdapterService FindAdapterService(const String& serviceUri); + +/// check for loaded eCAP services without matching ecap_service in squid.conf +extern void CheckUnusedAdapterServices(const Services& services); } // namespace Ecap } // namespace Adaptation diff --git a/src/adaptation/icap/Config.cc b/src/adaptation/icap/Config.cc index e6f6ba8ddd..06abc33356 100644 --- a/src/adaptation/icap/Config.cc +++ b/src/adaptation/icap/Config.cc @@ -59,9 +59,7 @@ Adaptation::Icap::Config::~Config() Adaptation::ServicePointer Adaptation::Icap::Config::createService(const Adaptation::ServiceConfig &cfg) { - Adaptation::Icap::ServiceRep::Pointer s = new Adaptation::Icap::ServiceRep(cfg); - s->setSelf(s); - return s.getRaw(); + return new Adaptation::Icap::ServiceRep(cfg); } time_t Adaptation::Icap::Config::connect_timeout(bool bypassable) const diff --git a/src/adaptation/icap/ServiceRep.cc b/src/adaptation/icap/ServiceRep.cc index c6a0740050..1a621f8c51 100644 --- a/src/adaptation/icap/ServiceRep.cc +++ b/src/adaptation/icap/ServiceRep.cc @@ -19,28 +19,21 @@ Adaptation::Icap::ServiceRep::ServiceRep(const Adaptation::ServiceConfig &svcCfg AsyncJob("Adaptation::Icap::ServiceRep"), Adaptation::Service(svcCfg), theOptions(NULL), theOptionsFetcher(0), theLastUpdate(0), theSessionFailures(0), isSuspended(0), notifying(false), - updateScheduled(false), self(NULL), - wasAnnouncedUp(true) // do not announce an "up" service at startup + updateScheduled(false), + wasAnnouncedUp(true), // do not announce an "up" service at startup + isDetached(false) {} Adaptation::Icap::ServiceRep::~ServiceRep() { Must(!theOptionsFetcher); - changeOptions(0); -} - -void -Adaptation::Icap::ServiceRep::setSelf(Pointer &aSelf) -{ - assert(!self && aSelf != NULL); - self = aSelf; + delete theOptions; } void Adaptation::Icap::ServiceRep::finalize() { Adaptation::Service::finalize(); - assert(self != NULL); // use /etc/services or default port if needed const bool have_port = cfg().port >= 0; @@ -55,18 +48,6 @@ Adaptation::Icap::ServiceRep::finalize() } } -void Adaptation::Icap::ServiceRep::invalidate() -{ - assert(self != NULL); - Pointer savedSelf = self; // to prevent destruction when we nullify self - self = NULL; - - announceStatusChange("invalidated by reconfigure", false); - - savedSelf = NULL; // may destroy us and, hence, invalidate cbdata(this) - // TODO: it would be nice to invalidate cbdata(this) when not destroyed -} - void Adaptation::Icap::ServiceRep::noteFailure() { ++theSessionFailures; @@ -110,7 +91,7 @@ bool Adaptation::Icap::ServiceRep::hasOptions() const bool Adaptation::Icap::ServiceRep::up() const { - return self != NULL && !isSuspended && hasOptions(); + return !isSuspended && hasOptions(); } bool Adaptation::Icap::ServiceRep::wantsUrl(const String &urlPath) const @@ -151,10 +132,10 @@ void ServiceRep_noteTimeToUpdate(void *data) void Adaptation::Icap::ServiceRep::noteTimeToUpdate() { - if (self != NULL) + if (!detached()) updateScheduled = false; - if (!self || theOptionsFetcher) { + if (detached() || theOptionsFetcher) { debugs(93,5, HERE << "ignores options update " << status()); return; } @@ -200,11 +181,10 @@ void Adaptation::Icap::ServiceRep::callWhenReady(AsyncCall::Pointer &cb) debugs(93,5, HERE << "Adaptation::Icap::Service is asked to call " << *cb << " when ready " << status()); - Must(self != NULL); Must(!broken()); // we do not wait for a broken service Client i; - i.service = self; // TODO: is this really needed? + i.service = Pointer(this); // TODO: is this really needed? i.callback = cb; theClients.push_back(i); @@ -225,7 +205,7 @@ void Adaptation::Icap::ServiceRep::scheduleNotification() bool Adaptation::Icap::ServiceRep::needNewOptions() const { - return self != NULL && !up(); + return !detached() && !up(); } void Adaptation::Icap::ServiceRep::changeOptions(Adaptation::Icap::Options *newOptions) @@ -444,8 +424,6 @@ const char *Adaptation::Icap::ServiceRep::status() const buf.append("up", 2); else { buf.append("down", 4); - if (!self) - buf.append(",gone", 5); if (isSuspended) buf.append(",susp", 5); @@ -457,6 +435,9 @@ const char *Adaptation::Icap::ServiceRep::status() const buf.append(",stale", 6); } + if (detached()) + buf.append(",detached", 9); + if (theOptionsFetcher) buf.append(",fetch", 6); @@ -471,3 +452,15 @@ const char *Adaptation::Icap::ServiceRep::status() const return buf.content(); } + +void Adaptation::Icap::ServiceRep::detach() +{ + debugs(93,3, HERE << "detaching ICAP service: " << cfg().uri << + ' ' << status()); + isDetached = true; +} + +bool Adaptation::Icap::ServiceRep::detached() const +{ + return isDetached; +} diff --git a/src/adaptation/icap/ServiceRep.h b/src/adaptation/icap/ServiceRep.h index 0bbaaa5a1f..23d76443a6 100644 --- a/src/adaptation/icap/ServiceRep.h +++ b/src/adaptation/icap/ServiceRep.h @@ -89,13 +89,10 @@ public: ServiceRep(const Adaptation::ServiceConfig &config); virtual ~ServiceRep(); - void setSelf(Pointer &aSelf); // needs self pointer for OptXact virtual void finalize(); - void invalidate(); // call when the service is no longer needed or valid - - bool probed() const; // see comments above - bool up() const; // see comments above + virtual bool probed() const; // see comments above + virtual bool up() const; // see comments above virtual Adaptation::Initiate *makeXactLauncher(Adaptation::Initiator *, HttpMsg *virginHeader, HttpRequest *virginCause); @@ -111,6 +108,9 @@ public: //AsyncJob virtual methods virtual bool doneAll() const { return Adaptation::Initiator::doneAll() && false;} + virtual void detach(); + virtual bool detached() const; + public: // treat these as private, they are for callbacks only void noteTimeToUpdate(); void noteTimeToNotify(); @@ -163,8 +163,8 @@ private: const char *status() const; - Pointer self; mutable bool wasAnnouncedUp; // prevent sequential same-state announcements + bool isDetached; CBDATA_CLASS2(ServiceRep); }; diff --git a/src/main.cc b/src/main.cc index 876814f1bd..162b04d00e 100644 --- a/src/main.cc +++ b/src/main.cc @@ -736,6 +736,24 @@ mainReconfigureFinish(void *) parseEtcHosts(); errorInitialize(); /* reload error pages */ accessLogInit(); + +#if USE_LOADABLE_MODULES + LoadableModulesConfigure(Config.loadable_module_names); +#endif + +#if USE_ADAPTATION + bool enableAdaptation = false; +#if ICAP_CLIENT + Adaptation::Icap::TheConfig.finalize(); + enableAdaptation = Adaptation::Icap::TheConfig.onoff || enableAdaptation; +#endif +#if USE_ECAP + Adaptation::Ecap::TheConfig.finalize(); // must be after we load modules + enableAdaptation = Adaptation::Ecap::TheConfig.onoff || enableAdaptation; +#endif + Adaptation::Config::Finalize(enableAdaptation); +#endif + #if ICAP_CLIENT icapLogOpen(); #endif -- 2.47.3