]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Author: Alex Rousskov <rousskov@measurement-factory.com>
authorAmos Jeffries <amosjeffries@squid-cache.org>
Fri, 28 May 2010 06:10:57 +0000 (00:10 -0600)
committerAmos Jeffries <amosjeffries@squid-cache.org>
Fri, 28 May 2010 06:10:57 +0000 (00:10 -0600)
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.

12 files changed:
src/adaptation/Config.cc
src/adaptation/Service.cc
src/adaptation/Service.h
src/adaptation/ecap/Config.cc
src/adaptation/ecap/Host.cc
src/adaptation/ecap/Host.h
src/adaptation/ecap/ServiceRep.cc
src/adaptation/ecap/ServiceRep.h
src/adaptation/icap/Config.cc
src/adaptation/icap/ServiceRep.cc
src/adaptation/icap/ServiceRep.h
src/main.cc

index e7f9332e8cf82df1cccc0d85619f2b40804dccad..2bdce463f1ee6883da12941de131b6187b942811 100644 (file)
@@ -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();
 }
index 4ce90e8e2b266ed244e23e24f12301fcd7c25210..7b9729a2214fb49fd5d718af62d709ca4e49323f 100644 (file)
@@ -68,3 +68,9 @@ Adaptation::FindService(const Service::Id& key)
     }
     return NULL;
 }
+
+void Adaptation::DetachServices()
+{
+    while (!AllServices().empty())
+        AllServices().pop_back()->detach();
+}
index 99a15bb1f12241b9d5aad691e841fdede54445d0..ab770d9fcd0c9d9b620dc8c7d3f88dd1a7dc1296 100644 (file)
@@ -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<Adaptation::ServicePointer> 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 */
index 70222f951b72fad4bcb6df94d62edab8a02436fc..c6ed5df8ca7246c108ce2c7b78220c584c98ee14 100644 (file)
@@ -4,7 +4,6 @@
  */
 #include "squid.h"
 
-#include <libecap/common/registry.h>
 #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<Adaptation::Ecap::Host> host(new Adaptation::Ecap::Host);
-    libecap::RegisterHost(host);
+    Host::Register();
+    CheckUnusedAdapterServices(AllServices());
 }
 
 Adaptation::ServicePointer
index 0c24801eff4321a75157636f643abbe09d8f88f1..b98902d0012241ba1c8b1d2be60b4d480c32fc90 100644 (file)
@@ -4,6 +4,7 @@
 #include "squid.h"
 #include <libecap/adapter/service.h>
 #include <libecap/common/names.h>
+#include <libecap/common/registry.h>
 #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<Adaptation::Ecap::Host> 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<libecap::adapter::Service> &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<libecap::adapter::Service> 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<ServiceRep*>(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);
+    }
+}
index 85f257bf55b94081032bc6f37a226aa5679cbbc0..8d5f32fa02cbbda221cd177e8ed1d9a5f587a052 100644 (file)
@@ -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;
index e4188306c6637849bc6d9f493276e516aa88a6e7..0961cb84103d2d2f988f1d3561159e8ed8a4f226 100644 (file)
@@ -2,13 +2,18 @@
  * DEBUG: section 93    eCAP Interface
  */
 #include "squid.h"
+#include <list>
 #include <libecap/adapter/service.h>
 #include "TextException.h"
 #include "adaptation/ecap/ServiceRep.h"
 #include "adaptation/ecap/XactionRep.h"
 
+// configured eCAP service wrappers
+static std::list<Adaptation::Ecap::ServiceRep::AdapterService> 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<ServiceRep::AdapterService>::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<ServiceRep::AdapterService>::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<ServiceRep::AdapterService>::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<ServiceRep::AdapterService>::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());
+    }
 }
index ed4c7d376ca3a6a4f705afce25a344531df2814c..27046b9a98f1af0ff6854480b0c37a17533944f2 100644 (file)
@@ -27,13 +27,9 @@ public:
     virtual ~ServiceRep();
 
     typedef libecap::shared_ptr<libecap::adapter::Service> 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
 
index e6f6ba8dddd51e3f3de7031e375e0007973efec9..06abc33356342f64e0baa48db2bf73120a5a73cf 100644 (file)
@@ -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
index c6a0740050a9bec5aaa3ae6d0c5ca65ff2214f1a..1a621f8c518ebcd0f762d88b2c391e76f2842169 100644 (file)
@@ -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;
+}
index 0bbaaa5a1ff6505e33f2f6f97067d2e6aac06fe6..23d76443a61a2e86c4858b16cd5cef689a3f2e79 100644 (file)
@@ -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);
 };
 
index 876814f1bd04a074b5d7273e23d34255ec29b398..162b04d00e38b0d08ca34bb5e3efa672a8748539 100644 (file)
@@ -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