From c0ee432598eccaeb46dab78194cf33ac5a0e38f1 Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Thu, 10 Dec 2015 17:31:18 -0500 Subject: [PATCH] Allow ChannelProviderFactory::newInstance to accept a Configuration Deprecate ChannelProvider::configure(), which doesn't do much and is incompatible with the idea of shared context. A lot of down-stream mess related to the confused relationship between InternalClientContextImpl and InternalClientContextImpl::ChannelProviderImpl. This is changed to compose the provider within the context and use a nested shared_ptr so that references to the provider are really references to the context. This brings the ownership semantic in line with what the API suggests, and what other providers implement. --- src/client/pvAccess.h | 19 ++- src/pva/clientFactory.cpp | 70 ++++------- src/remoteClient/clientContextImpl.cpp | 159 ++++++++---------------- src/remoteClient/clientContextImpl.h | 6 +- testApp/remote/testRemoteClientImpl.cpp | 11 -- 5 files changed, 95 insertions(+), 170 deletions(-) diff --git a/src/client/pvAccess.h b/src/client/pvAccess.h index c8afdf0..f7f275c 100644 --- a/src/client/pvAccess.h +++ b/src/client/pvAccess.h @@ -32,6 +32,7 @@ namespace epics { namespace pvAccess { + class Configuration; // TODO add write-only? // change names @@ -870,7 +871,7 @@ namespace pvAccess { virtual Channel::shared_pointer createChannel(std::string const & channelName,ChannelRequester::shared_pointer const & channelRequester, short priority, std::string const & address) = 0; - virtual void configure(epics::pvData::PVStructure::shared_pointer /*configuration*/) {}; + virtual void configure(epics::pvData::PVStructure::shared_pointer /*configuration*/) EPICS_DEPRECATED {}; virtual void flush() {}; virtual void poll() {}; @@ -892,16 +893,26 @@ namespace pvAccess { virtual std::string getFactoryName() = 0; /** - * Get a shared instance. + * Get a shared instance using the default Configuration. * @return a shared instance. */ virtual ChannelProvider::shared_pointer sharedInstance() = 0; /** - * Create a new instance. + * Create a new instance using the default Configuration. * @return a new instance. */ - virtual ChannelProvider::shared_pointer newInstance() = 0; + virtual ChannelProvider::shared_pointer newInstance() { + return newInstance(std::tr1::shared_ptr()); + } + + /** + * Create a new instance using a specific Configuration. + * @return a new instance. + */ + virtual ChannelProvider::shared_pointer newInstance(const std::tr1::shared_ptr&) { + throw std::logic_error("This ChannelProviderFactory does not support non-default configurations"); + } }; /** diff --git a/src/pva/clientFactory.cpp b/src/pva/clientFactory.cpp index abac587..3157b91 100644 --- a/src/pva/clientFactory.cpp +++ b/src/pva/clientFactory.cpp @@ -16,9 +16,8 @@ using namespace epics::pvData; using namespace epics::pvAccess; -// TODO global static variable (de/initialization order not guaranteed) -static Mutex mutex; -static ClientContextImpl::shared_pointer context; +static Mutex cfact_mutex; +static ChannelProvider::shared_pointer cfact_shared_provider; class ChannelProviderFactoryImpl : public ChannelProviderFactory { @@ -32,70 +31,47 @@ public: virtual ChannelProvider::shared_pointer sharedInstance() { - Lock guard(mutex); - if (!context.get()) + Lock guard(cfact_mutex); + if (!cfact_shared_provider) { - try { - ClientContextImpl::shared_pointer lcontext = createClientContextImpl(); - lcontext->initialize(); - context = lcontext; - } catch (std::exception &e) { - LOG(logLevelError, "Unhandled exception caught at %s:%d: %s", __FILE__, __LINE__, e.what()); - } catch (...) { - LOG(logLevelError, "Unhandled exception caught at %s:%d.", __FILE__, __LINE__); - } + Configuration::shared_pointer def; + cfact_shared_provider = createClientProvider(def); } - return context->getProvider(); + return cfact_shared_provider; } - virtual ChannelProvider::shared_pointer newInstance() + virtual ChannelProvider::shared_pointer newInstance(const std::tr1::shared_ptr& conf) { - Lock guard(mutex); - try { - ClientContextImpl::shared_pointer lcontext = createClientContextImpl(); - lcontext->initialize(); - return lcontext->getProvider(); - } catch (std::exception &e) { - LOG(logLevelError, "Unhandled exception caught at %s:%d: %s", __FILE__, __LINE__, e.what()); - return ChannelProvider::shared_pointer(); - } catch (...) { - LOG(logLevelError, "Unhandled exception caught at %s:%d.", __FILE__, __LINE__); - return ChannelProvider::shared_pointer(); - } - } - - void destroySharedInstance() - { - Lock guard(mutex); - if (context.get()) - { - context->dispose(); - context.reset(); - } + Lock guard(cfact_mutex); + return createClientProvider(conf); } }; -static ChannelProviderFactoryImpl::shared_pointer factory; +static ChannelProviderFactoryImpl::shared_pointer pva_factory; void ClientFactory::start() { epicsSignalInstallSigAlarmIgnore(); epicsSignalInstallSigPipeIgnore(); - Lock guard(mutex); - if (!factory.get()) - factory.reset(new ChannelProviderFactoryImpl()); + Lock guard(cfact_mutex); + if (!pva_factory) + pva_factory.reset(new ChannelProviderFactoryImpl()); - registerChannelProviderFactory(factory); + registerChannelProviderFactory(pva_factory); } void ClientFactory::stop() { - Lock guard(mutex); + Lock guard(cfact_mutex); - if (factory.get()) + if (pva_factory) { - unregisterChannelProviderFactory(factory); - factory->destroySharedInstance(); + unregisterChannelProviderFactory(pva_factory); + if(!pva_factory.unique()) { + LOG(logLevelWarn, "ClientFactory::stop() finds shared client context with %u remaining users", + (unsigned)pva_factory.use_count()); + } + pva_factory.reset(); } } diff --git a/src/remoteClient/clientContextImpl.cpp b/src/remoteClient/clientContextImpl.cpp index 84a2e66..a64dfdc 100644 --- a/src/remoteClient/clientContextImpl.cpp +++ b/src/remoteClient/clientContextImpl.cpp @@ -3269,45 +3269,12 @@ namespace epics { public ClientContextImpl, public std::tr1::enable_shared_from_this { - - - class ChannelProviderImpl; -/* - class ChannelImplFind : public ChannelFind - { - public: - ChannelImplFind(ChannelProvider::shared_pointer const & provider) : m_provider(provider) - { - } - - virtual void destroy() - { - // one instance for all, do not delete at all - } - - virtual ChannelProvider::shared_pointer getChannelProvider() - { - return m_provider; - }; - - virtual void cancel() - { - throw std::runtime_error("not supported"); - } - - private: - - // only to be destroyed by it - friend class ChannelProviderImpl; - virtual ~ChannelImplFind() {} - - ChannelProvider::shared_pointer m_provider; - }; -*/ + public: + POINTER_DEFINITIONS(InternalClientContextImpl); class ChannelProviderImpl : public ChannelProvider { public: - ChannelProviderImpl(std::tr1::shared_ptr const & context) : + ChannelProviderImpl(InternalClientContextImpl* context) : m_context(context) { MB_INIT; @@ -3328,9 +3295,7 @@ namespace epics { { // TODO not implemented - std::tr1::shared_ptr context = m_context.lock(); - if (context.get()) - context->checkChannelName(channelName); + m_context->checkChannelName(channelName); if (!channelFindRequester.get()) throw std::runtime_error("null requester"); @@ -3368,15 +3333,6 @@ namespace epics { short priority, std::string const & addressesStr) { - std::tr1::shared_ptr context = m_context.lock(); - if (!context.get()) - { - Status errorStatus(Status::STATUSTYPE_ERROR, "context already destroyed"); - Channel::shared_pointer nullChannel; - EXCEPTION_GUARD(channelRequester->channelCreated(errorStatus, nullChannel)); - return nullChannel; - } - auto_ptr addresses; if (!addressesStr.empty()) { @@ -3385,7 +3341,7 @@ namespace epics { addresses.reset(); } - Channel::shared_pointer channel = context->createChannelInternal(channelName, channelRequester, priority, addresses); + Channel::shared_pointer channel = m_context->createChannelInternal(channelName, channelRequester, priority, addresses); if (channel.get()) channelRequester->channelCreated(Status::Ok, channel); return channel; @@ -3395,37 +3351,31 @@ namespace epics { virtual void configure(epics::pvData::PVStructure::shared_pointer configuration) { - std::tr1::shared_ptr context = m_context.lock(); - if (context.get()) - context->configure(configuration); + LOG(logLevelError, "Client context must be configured at construction (see ChannelProvider::newInstance(conf))"); } virtual void flush() { - std::tr1::shared_ptr context = m_context.lock(); - if (context.get()) - context->flush(); + m_context->flush(); } virtual void poll() { - std::tr1::shared_ptr context = m_context.lock(); - if (context.get()) - context->poll(); + m_context->poll(); } ~ChannelProviderImpl() {}; private: - std::tr1::weak_ptr m_context; + InternalClientContextImpl* const m_context; }; - + private: /** * Implementation of PVAJ JCA Channel. */ @@ -3439,7 +3389,7 @@ namespace epics { /** * Context. */ - ClientContextImpl::shared_pointer m_context; + std::tr1::shared_ptr m_context; /** * Client channel ID. @@ -3528,7 +3478,7 @@ namespace epics { * @throws PVAException */ InternalChannelImpl( - ClientContextImpl::shared_pointer const & context, + InternalClientContextImpl::shared_pointer const & context, pvAccessID channelID, string const & name, ChannelRequester::shared_pointer const & requester, @@ -3562,7 +3512,7 @@ namespace epics { public: - static ChannelImpl::shared_pointer create(ClientContextImpl::shared_pointer context, + static ChannelImpl::shared_pointer create(InternalClientContextImpl::shared_pointer context, pvAccessID channelID, string const & name, ChannelRequester::shared_pointer requester, @@ -3602,7 +3552,7 @@ namespace epics { virtual ChannelProvider::shared_pointer getProvider() { - return m_context->getProvider(); + return ChannelProvider::shared_pointer(m_context->m_provider_ptr); } // NOTE: synchronization guarantees that transport is non-0 and state == CONNECTED. @@ -3610,12 +3560,11 @@ namespace epics { { Lock guard(m_channelMutex); if (m_connectionState != CONNECTED) { - static string emptyString; - return emptyString; + return ""; } else { - return inetAddressToString(*m_transport->getRemoteAddress()); + return m_transport->getRemoteName(); } } @@ -4302,11 +4251,9 @@ namespace epics { - + public: - private: - - InternalClientContextImpl() : + InternalClientContextImpl(const Configuration::shared_pointer& conf) : m_addressList(""), m_autoAddressList(true), m_connectionTimeout(30.0f), m_beaconPeriod(15.0f), m_broadcastPort(PVA_BROADCAST_PORT), m_receiveBufferSize(MAX_TCP_RECV), m_namedLocker(), m_lastCID(0), m_lastIOID(0), @@ -4315,40 +4262,18 @@ namespace epics { EPICS_PVA_MINOR_VERSION, EPICS_PVA_MAINTENANCE_VERSION, EPICS_PVA_DEVELOPMENT_FLAG), + m_provider(this), m_contextState(CONTEXT_NOT_INITIALIZED), - m_configuration(new SystemConfigurationImpl()), + m_configuration(conf), m_flushStrategy(DELAYED) { PVACCESS_REFCOUNT_MONITOR_CONSTRUCT(remoteClientContext); + if(!m_configuration) m_configuration = ConfigurationFactory::getConfiguration("pvAccess-client"); m_flushTransports.reserve(64); loadConfiguration(); } - public: - - static shared_pointer create() - { - // TODO use std::make_shared - std::tr1::shared_ptr tp(new InternalClientContextImpl(), delayed_destroyable_deleter()); - shared_pointer thisPointer = tp; - static_cast(thisPointer.get())->activate(); - return thisPointer; - } - - void activate() - { - m_provider.reset(new ChannelProviderImpl(shared_from_this())); - } - virtual Configuration::shared_pointer getConfiguration() { - /* -TODO - final ConfigurationProvider configurationProvider = ConfigurationFactory.getProvider(); - Configuration config = configurationProvider.getConfiguration("pvAccess-client"); - if (!config) - config = configurationProvider.getConfiguration("system"); - return config; -*/ return m_configuration; } @@ -4356,10 +4281,6 @@ TODO return m_version; } - virtual ChannelProvider::shared_pointer const & getProvider() { - return m_provider; - } - virtual Timer::shared_pointer getTimer() { return m_timer; @@ -4916,7 +4837,7 @@ TODO } virtual void configure(epics::pvData::PVStructure::shared_pointer configuration) - { + {// remove? if (m_transportRegistry->numberOfActiveTransports() > 0) throw std::runtime_error("Configure must be called when there is no transports active."); @@ -5091,10 +5012,13 @@ TODO */ Version m_version; + public: /** * Provider implementation. */ - ChannelProviderImpl::shared_pointer m_provider; + ChannelProviderImpl m_provider; + ChannelProviderImpl::weak_pointer m_provider_ptr; + private: /** * Context state. @@ -5117,10 +5041,35 @@ TODO osiSockAddr m_localBroadcastAddress; }; - ClientContextImpl::shared_pointer createClientContextImpl() + namespace { + struct nested { + ClientContextImpl::shared_pointer ptr; + nested(const ClientContextImpl::shared_pointer& p) :ptr(p) {} + void operator()(ChannelProvider*) { + ptr.reset(); + } + }; + } + + ChannelProvider::shared_pointer createClientProvider(const Configuration::shared_pointer& conf) { - ClientContextImpl::shared_pointer ptr = InternalClientContextImpl::create(); - return ptr; + /* For PVA the context and provider have a tight 1-to-1 relationship + * were each must know about the other, and both must be referenced by shared_ptr + * from outside code (provider) and Channels (context and provider. + * + * So we compose the provider within the context and use a nested shared_ptr + * to the context for the provider. + */ + ClientContextImpl::shared_pointer ctxt(new InternalClientContextImpl(conf)); + + InternalClientContextImpl *self = static_cast(ctxt.get()); + + // a wrapped shared_ptr to the provider which really holds a reference to the context + ChannelProvider::shared_pointer prov(&self->m_provider, nested(ctxt)); + + self->m_provider_ptr = prov; // keep a weak_ptr for the context itself so that it can give out refs. to it's provider + ctxt->initialize(); + return prov; } }}; diff --git a/src/remoteClient/clientContextImpl.h b/src/remoteClient/clientContextImpl.h index 7843695..36c9a38 100644 --- a/src/remoteClient/clientContextImpl.h +++ b/src/remoteClient/clientContextImpl.h @@ -77,13 +77,13 @@ namespace epics { /** * Initialize client context. This method is called immediately after instance construction (call of constructor). */ - virtual void initialize() = 0; + virtual void initialize() = 0; // public? /** * Get channel provider implementation. * @return the channel provider. */ - virtual ChannelProvider::shared_pointer const & getProvider() = 0; + //virtual ChannelProvider::shared_pointer const & getProvider() = 0; /** * Prints detailed information about the context to the standard output stream. @@ -132,7 +132,7 @@ namespace epics { virtual const osiSockAddr& getLocalBroadcastAddress() const = 0; }; - epicsShareExtern ClientContextImpl::shared_pointer createClientContextImpl(); + epicsShareExtern ChannelProvider::shared_pointer createClientProvider(const Configuration::shared_pointer& conf); } } diff --git a/testApp/remote/testRemoteClientImpl.cpp b/testApp/remote/testRemoteClientImpl.cpp index dcb54e5..73ec41a 100644 --- a/testApp/remote/testRemoteClientImpl.cpp +++ b/testApp/remote/testRemoteClientImpl.cpp @@ -371,17 +371,6 @@ int main() { for (int i = 0; i < 10; i++) { { - /* - ClientContextImpl::shared_pointer context = createClientContextImpl(); - context->printInfo(); - - context->initialize(); - context->printInfo(); - - epicsThreadSleep ( SLEEP_TIME ); - - ChannelProvider::shared_pointer provider = context->getProvider(); - */ ClientFactory::start(); ChannelProvider::shared_pointer provider = getChannelProviderRegistry()->getProvider("pva");