From 0f8a3b2b7d642fb0073fc3bb27614cf67a1662c7 Mon Sep 17 00:00:00 2001 From: mrkraimer Date: Sun, 4 Jun 2017 07:49:28 -0400 Subject: [PATCH] changes explained in MRKCHANGES.md --- MRKCHANGES.md | 103 +++++++++++++++++++++++++++ src/ca/caProvider.cpp | 89 +++++++++++++---------- src/ca/pv/caProvider.h | 11 ++- src/client/pv/pvAccess.h | 38 ++++++++-- src/factory/ChannelAccessFactory.cpp | 55 ++++++++++---- src/pva/clientFactory.cpp | 45 ++++++++---- src/pva/pv/clientFactory.h | 12 ++++ 7 files changed, 281 insertions(+), 72 deletions(-) create mode 100644 MRKCHANGES.md diff --git a/MRKCHANGES.md b/MRKCHANGES.md new file mode 100644 index 0000000..b670790 --- /dev/null +++ b/MRKCHANGES.md @@ -0,0 +1,103 @@ +======== +Overview +======== + +pvaCientCPP did not terminate properly. +It has a singleton class named **PvaClient**. +When the singleton is created it calls: + + ClientFactory::start(); + +The destructor for **PvaClient** calls: + + ClientFactory::stop(); + +This call results in the following message: + + terminate called after throwing an instance of 'epicsMutex::invalidMutex' + what(): epicsMutex::invalidMutex() + +Further investigation found that this was caused by a mutex belonging to ChannelProviderRegistry. + +If fact it appeared that the only way the crash did not occur is if the client code was of the form: + + int main(int argc,char *argv[]) + { + ClientFactory::start(); + ... + ClientFactory::stop(); + } + +While investigating this problem I also realized that the existing implementation of +ClientFactory only worked if a single call was made to start and a single call to stop. + +In pvAccessCPP changes were made to the followimg: + +1) ChannelProviderRegistry +2) ClientFactory +3) CAChannelProviderFactory + +These are discussed below. + +Changes were also made to pvaClientCPP. +The change was to use the new ChannelProviderRegistry methods. + +All these changes are in the github repositories belonging to mrkraimer. + +----------------------- +ChannelProviderRegistry +----------------------- + +The following new methods are added to ChannelProviderRegistry: + + + static ChannelProviderRegistry::shared_pointer getChannelProviderRegistry(); + void registerChannelProviderFactory( + ChannelProviderFactory::shared_pointer const & channelProviderFactory); + void unregisterChannelProviderFactory( + ChannelProviderFactory::shared_pointer const & channelProviderFactory); + +**getChannelProviderRegistry** creates the single instance of **ChannelProviderRegistry** +the first time it is called and always returns a shared pointer to the single +instance, + +Any facility that calls this must keep the shared pointer until the faciliity no longer requires +use of the registry or any channel provider the facliity uses. + +The above methods replace the following functions: + + // THE FOLLOWING SHOULD NOT BE CALLED + epicsShareFunc ChannelProviderRegistry::shared_pointer getChannelProviderRegistry() EPICS_DEPRECATED; + epicsShareFunc void registerChannelProviderFactory( + ChannelProviderFactory::shared_pointer const & channelProviderFactory) EPICS_DEPRECATED; + epicsShareFunc void unregisterChannelProviderFactory( + ChannelProviderFactory::shared_pointer const & channelProviderFactory) EPICS_DEPRECATED; + epicsShareFunc void unregisterAllChannelProviderFactory() EPICS_DEPRECATED; + +Note that the methods are all deprecated. +They were kept so that existing code will still compile and run. +Any code that uses these methods may see the invalidMutex exception on termination. + +------------- +ClientFactory +------------- + +This has two major changes: + +1) It uses the new **ChannelProviderRegistry** methods, + +2) It allows multiple calls to start and stop. + + +------------------------ +CAChannelProviderFactory +------------------------ + +This has two major changes: + +1) It uses the new **ChannelProviderRegistry** methods, + +2) It allows multiple calls to start and stop. + + + diff --git a/src/ca/caProvider.cpp b/src/ca/caProvider.cpp index f3765ef..ecd8025 100644 --- a/src/ca/caProvider.cpp +++ b/src/ca/caProvider.cpp @@ -15,9 +15,12 @@ #include #include +namespace epics { +namespace pvAccess { +namespace ca { + using namespace epics::pvData; using namespace epics::pvAccess; -using namespace epics::pvAccess::ca; #define EXCEPTION_GUARD(code) try { code; } \ catch (std::exception &e) { LOG(logLevelError, "Unhandled exception caught from client code at %s:%d: %s", __FILE__, __LINE__, e.what()); } \ @@ -165,23 +168,25 @@ void CAChannelProvider::initialize() } - - - - - - - - - -// TODO global static variable (de/initialization order not guaranteed) -static Mutex mutex; -static CAChannelProvider::shared_pointer sharedProvider; - -class CAChannelProviderFactoryImpl : public ChannelProviderFactory +class CAChannelProviderFactory : public ChannelProviderFactory { +private: + Mutex m_mutex; + CAChannelProvider::shared_pointer sharedProvider; public: - POINTER_DEFINITIONS(CAChannelProviderFactoryImpl); + POINTER_DEFINITIONS(CAChannelProviderFactory); + + virtual ~CAChannelProviderFactory() + { + Lock guard(m_mutex); + if (sharedProvider) + { + CAChannelProvider::shared_pointer provider; + sharedProvider.swap(provider); + // factroy cleans up also shared provider + provider->destroy(); + } + } virtual std::string getFactoryName() { @@ -190,8 +195,8 @@ public: virtual ChannelProvider::shared_pointer sharedInstance() { - Lock guard(mutex); - if (!sharedProvider.get()) + Lock guard(m_mutex); + if (!sharedProvider) { try { // TODO use std::make_shared @@ -224,35 +229,47 @@ public: } } - void destroySharedInstance() - { - if(!sharedProvider) return; - sharedProvider->destroy(); - sharedProvider.reset(); - } }; -static CAChannelProviderFactoryImpl::shared_pointer factory; +static Mutex startStopMutex; + +ChannelProviderRegistry::shared_pointer CAClientFactory::channelRegistry = ChannelProviderRegistry::shared_pointer(); +CAChannelProviderFactoryPtr CAClientFactory::channelProvider = CAChannelProviderFactory::shared_pointer(); +int CAClientFactory::numStart = 0; + void CAClientFactory::start() { + Lock guard(startStopMutex); +std::cout << "CAClientFactory::start() numStart " << numStart << std::endl; + ++numStart; + if(numStart>1) return; epicsSignalInstallSigAlarmIgnore(); epicsSignalInstallSigPipeIgnore(); - - Lock guard(mutex); - if (!factory.get()) - factory.reset(new CAChannelProviderFactoryImpl()); - - registerChannelProviderFactory(factory); + channelProvider.reset(new CAChannelProviderFactory()); + channelRegistry = ChannelProviderRegistry::getChannelProviderRegistry(); +std::cout << "channelRegistry::use_count " << channelRegistry.use_count() << std::endl; + channelRegistry->registerChannelProviderFactory(channelProvider); } void CAClientFactory::stop() { - Lock guard(mutex); - - if (factory.get()) +std::cout << "ClientFactory::stop() numStart " << numStart << std::endl; +std::cout << "channelRegistry::use_count " << channelRegistry.use_count() << std::endl; + Lock guard(startStopMutex); + if(numStart==0) return; + --numStart; + if(numStart>=1) return; + if (channelProvider) { - unregisterChannelProviderFactory(factory); - factory->destroySharedInstance(); + channelRegistry->unregisterChannelProviderFactory(channelProvider); + if(!channelProvider.unique()) { + LOG(logLevelWarn, "ClientFactory::stop() finds shared client context with %u remaining users", + (unsigned)channelProvider.use_count()); + } + channelProvider.reset(); + channelRegistry.reset(); } } + +}}} diff --git a/src/ca/pv/caProvider.h b/src/ca/pv/caProvider.h index 775ded4..aa08fd8 100644 --- a/src/ca/pv/caProvider.h +++ b/src/ca/pv/caProvider.h @@ -19,7 +19,7 @@ namespace pvAccess { namespace ca { class epicsShareClass CAChannelProvider : - public ChannelProvider, + public epics::pvAccess::ChannelProvider, public std::tr1::enable_shared_from_this { public: @@ -82,9 +82,14 @@ private: bool destroyed; }; +class CAChannelProviderFactory; +typedef std::tr1::shared_ptr CAChannelProviderFactoryPtr; -class epicsShareClass CAClientFactory -{ +class epicsShareClass CAClientFactory { +private: + static epics::pvAccess::ChannelProviderRegistry::shared_pointer channelRegistry; + static CAChannelProviderFactoryPtr channelProvider; + static int numStart; public: static void start(); static void stop(); diff --git a/src/client/pv/pvAccess.h b/src/client/pv/pvAccess.h index d8430f7..afde893 100644 --- a/src/client/pv/pvAccess.h +++ b/src/client/pv/pvAccess.h @@ -998,8 +998,30 @@ class epicsShareClass ChannelProviderRegistry { public: POINTER_DEFINITIONS(ChannelProviderRegistry); + ~ChannelProviderRegistry(); + typedef std::vector stringVector_t; + /** + * Get a shared instance of the single instance of ChannelProviderRegistry. + * The caller must keep the shared instance until the caller no longer needs + * the registry. The first call creates the single instance. + * @return The interface for ChannelProviderRegistry + */ + static ChannelProviderRegistry::shared_pointer getChannelProviderRegistry(); + /** + * Register a ChannelProviderFactory. + * @param channelProviderFactory The ChannelProviderFactory. + */ + void registerChannelProviderFactory( + ChannelProviderFactory::shared_pointer const & channelProviderFactory); + /** + * Unregister a ChannelProviderFactory. + * @param channelProviderFactory The ChannelProviderFactory. + */ + void unregisterChannelProviderFactory( + ChannelProviderFactory::shared_pointer const & channelProviderFactory); + /** * Get a shared instance of the provider with the specified name. * @param providerName The name of the provider. @@ -1064,18 +1086,22 @@ public: } private: - ChannelProviderRegistry() {} +// ChannelProviderRegistry() {} + ChannelProviderRegistry(); + epics::pvData::Mutex mutex; typedef std::map providers_t; providers_t providers; }; -//! never returns NULL -epicsShareFunc ChannelProviderRegistry::shared_pointer getChannelProviderRegistry(); -epicsShareFunc void registerChannelProviderFactory(ChannelProviderFactory::shared_pointer const & channelProviderFactory); -epicsShareFunc void unregisterChannelProviderFactory(ChannelProviderFactory::shared_pointer const & channelProviderFactory); -epicsShareFunc void unregisterAllChannelProviderFactory(); +// THE FOLLOWING SHOULD NOT BE CALLED +epicsShareFunc ChannelProviderRegistry::shared_pointer getChannelProviderRegistry() EPICS_DEPRECATED; +epicsShareFunc void registerChannelProviderFactory( + ChannelProviderFactory::shared_pointer const & channelProviderFactory) EPICS_DEPRECATED; +epicsShareFunc void unregisterChannelProviderFactory( + ChannelProviderFactory::shared_pointer const & channelProviderFactory) EPICS_DEPRECATED; +epicsShareFunc void unregisterAllChannelProviderFactory() EPICS_DEPRECATED; /** diff --git a/src/factory/ChannelAccessFactory.cpp b/src/factory/ChannelAccessFactory.cpp index 97f0a8d..4ef4ad1 100644 --- a/src/factory/ChannelAccessFactory.cpp +++ b/src/factory/ChannelAccessFactory.cpp @@ -21,6 +21,42 @@ using std::string; namespace epics { namespace pvAccess { +ChannelProviderRegistry::ChannelProviderRegistry() +{ +std::cout << "ChannelProviderRegistry\n"; +} + +ChannelProviderRegistry::~ChannelProviderRegistry() +{ +std::cout << "~ChannelProviderRegistry\n"; +} + + +ChannelProviderRegistry::shared_pointer ChannelProviderRegistry::getChannelProviderRegistry() +{ + static Mutex mutex; + static ChannelProviderRegistry::shared_pointer global_reg; + Lock guard(mutex); + if(!global_reg) { + global_reg = ChannelProviderRegistry::build(); + } + return global_reg; +} + +void ChannelProviderRegistry::registerChannelProviderFactory( + ChannelProviderFactory::shared_pointer const & channelProviderFactory) +{ + assert(channelProviderFactory); + add(channelProviderFactory); +} + +void ChannelProviderRegistry::unregisterChannelProviderFactory( + ChannelProviderFactory::shared_pointer const & channelProviderFactory) +{ + assert(channelProviderFactory); + remove(channelProviderFactory->getFactoryName()); +} + ChannelProvider::shared_pointer ChannelProviderRegistry::getProvider(std::string const & providerName) { ChannelProviderFactory::shared_pointer fact(getFactory(providerName)); if(fact) @@ -78,6 +114,7 @@ bool ChannelProviderRegistry::add(const ChannelProviderFactory::shared_pointer& } ChannelProviderFactory::shared_pointer ChannelProviderRegistry::remove(const std::string& name) + { ChannelProviderFactory::shared_pointer ret; providers_t::iterator iter(providers.find(name)); @@ -89,24 +126,18 @@ ChannelProviderFactory::shared_pointer ChannelProviderRegistry::remove(const std } ChannelProviderRegistry::shared_pointer getChannelProviderRegistry() { - static Mutex mutex; - static ChannelProviderRegistry::shared_pointer global_reg; - Lock guard(mutex); - - if(!global_reg) { - global_reg = ChannelProviderRegistry::build(); - } - return global_reg; +std::cerr << "getChannelProviderRegistry should not be used\n"; + return ChannelProviderRegistry::getChannelProviderRegistry(); } void registerChannelProviderFactory(ChannelProviderFactory::shared_pointer const & channelProviderFactory) { - assert(channelProviderFactory); - getChannelProviderRegistry()->add(channelProviderFactory); +std::cerr << "registerChannelProviderFactory should not be used\n"; + getChannelProviderRegistry()->registerChannelProviderFactory(channelProviderFactory); } void unregisterChannelProviderFactory(ChannelProviderFactory::shared_pointer const & channelProviderFactory) { - assert(channelProviderFactory); - getChannelProviderRegistry()->remove(channelProviderFactory->getFactoryName()); +std::cerr << "unregisterChannelProviderFactory should not be used\n"; + getChannelProviderRegistry()->unregisterChannelProviderFactory(channelProviderFactory); } epicsShareFunc void unregisterAllChannelProviderFactory() diff --git a/src/pva/clientFactory.cpp b/src/pva/clientFactory.cpp index 9075e70..ef95f0f 100644 --- a/src/pva/clientFactory.cpp +++ b/src/pva/clientFactory.cpp @@ -15,8 +15,10 @@ #include #include +namespace epics { +namespace pvAccess { + using namespace epics::pvData; -using namespace epics::pvAccess; class ChannelProviderFactoryImpl : public ChannelProviderFactory { @@ -62,32 +64,45 @@ public: } }; -static Mutex cprovfact_mutex; -static ChannelProviderFactoryImpl::shared_pointer pva_factory; +static Mutex startStopMutex; + +ChannelProviderRegistryPtr ClientFactory::channelRegistry = ChannelProviderRegistryPtr(); +ChannelProviderFactoryImplPtr ClientFactory::channelProvider = ChannelProviderFactoryImplPtr(); +int ClientFactory::numStart = 0; void ClientFactory::start() { + Lock guard(startStopMutex); +std::cout << "ClientFactory::start() numStart " << numStart << std::endl; + ++numStart; + if(numStart>1) return; epicsSignalInstallSigAlarmIgnore(); epicsSignalInstallSigPipeIgnore(); - - Lock guard(cprovfact_mutex); - if (!pva_factory) - pva_factory.reset(new ChannelProviderFactoryImpl()); - - registerChannelProviderFactory(pva_factory); + channelProvider.reset(new ChannelProviderFactoryImpl()); + channelRegistry = ChannelProviderRegistry::getChannelProviderRegistry(); +std::cout << "channelRegistry::use_count " << channelRegistry.use_count() << std::endl; + channelRegistry->registerChannelProviderFactory(channelProvider); } void ClientFactory::stop() { - Lock guard(cprovfact_mutex); +std::cout << "ClientFactory::stop() numStart " << numStart << std::endl; +std::cout << "channelRegistry::use_count " << channelRegistry.use_count() << std::endl; + Lock guard(startStopMutex); + if(numStart==0) return; + --numStart; + if(numStart>=1) return; - if (pva_factory) + if (channelProvider) { - unregisterChannelProviderFactory(pva_factory); - if(!pva_factory.unique()) { + channelRegistry->unregisterChannelProviderFactory(channelProvider); + if(!channelProvider.unique()) { LOG(logLevelWarn, "ClientFactory::stop() finds shared client context with %u remaining users", - (unsigned)pva_factory.use_count()); + (unsigned)channelProvider.use_count()); } - pva_factory.reset(); + channelProvider.reset(); + channelRegistry.reset(); } } + +}} diff --git a/src/pva/pv/clientFactory.h b/src/pva/pv/clientFactory.h index 0261b76..65e387c 100644 --- a/src/pva/pv/clientFactory.h +++ b/src/pva/pv/clientFactory.h @@ -9,10 +9,22 @@ #include +#include + namespace epics { namespace pvAccess { +class ChannelProviderRegistry; +typedef std::tr1::shared_ptr ChannelProviderRegistryPtr; + +class ChannelProviderFactoryImpl; +typedef std::tr1::shared_ptr ChannelProviderFactoryImplPtr; + class epicsShareClass ClientFactory { +private: + static ChannelProviderRegistryPtr channelRegistry; + static ChannelProviderFactoryImplPtr channelProvider; + static int numStart; public: static void start(); static void stop();