]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Author: Alex Rousskov <rousskov@measurement-factory.com>
authorAmos Jeffries <squid3@treenet.co.nz>
Thu, 27 May 2010 00:51:44 +0000 (12:51 +1200)
committerAmos Jeffries <squid3@treenet.co.nz>
Thu, 27 May 2010 00:51:44 +0000 (12:51 +1200)
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 6137807fcd92f971a0a5d071e776ea8385a63259..50a727c7ae2d2eeca2c74e5e90ffb4b0bf17a4ef 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
@@ -48,6 +45,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; }
 
@@ -61,6 +64,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 53ee6fdd3c3b4ee7e2bab6cf11da36030878f9cc..9efe3ba05cedf44f9466d8856f0d1d61ae616ad5 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 "base/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 abfc45f116ba3d6dad27edf987834597b16916a7..438392dc1f23bf64208a91a4ece0c4dac2618bb1 100644 (file)
@@ -2,13 +2,18 @@
  * DEBUG: section 93    eCAP Interface
  */
 #include "squid.h"
+#include <list>
 #include <libecap/adapter/service.h>
 #include "adaptation/ecap/ServiceRep.h"
 #include "adaptation/ecap/XactionRep.h"
 #include "base/TextException.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 40212c2a2418dc1140306f9a6caef0f2d69ecf9d..ee96ffe35462f91ed8594e9f512cc5c4568f662f 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),
         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;
@@ -58,18 +51,6 @@ Adaptation::Icap::ServiceRep::finalize()
                                  TheConfig.oldest_service_failure : -1);
 }
 
-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()
 {
     const int failures = theSessionFailures.count(1);
@@ -114,7 +95,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
@@ -155,10 +136,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;
     }
@@ -204,11 +185,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);
 
@@ -229,7 +209,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)
@@ -448,8 +428,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);
 
@@ -461,6 +439,9 @@ const char *Adaptation::Icap::ServiceRep::status() const
             buf.append(",stale", 6);
     }
 
+    if (detached())
+        buf.append(",detached", 9);
+
     if (theOptionsFetcher)
         buf.append(",fetch", 6);
 
@@ -475,3 +456,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 3d955e39a92db172865f03a8a6c89c7ff1c19aa2..4bbd38a657049ea7e912bfb1d40341437f4f5d3f 100644 (file)
@@ -90,13 +90,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);
 
@@ -112,6 +109,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 3f57ae7ac97a48da8c2f1739c08efff02e7a8341..cc20480c0c79e8dd453bcf8f3f45f54946546cc2 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