From 1a100a095513e76945426d5781a7506ef5bac28f Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Wed, 28 Mar 2018 13:21:46 -0700 Subject: [PATCH] client fix init order leading to *NULL Avoid a race between the main thread alloc of SimpleChannelSearchManagerImpl the the UDP socket worker trying to use the same. Split allocation and initialization of search manager around socket creation and worker start. Also in-line initializeUDPTransport() to only call site. --- .../pv/simpleChannelSearchManagerImpl.h | 9 +-- src/remote/simpleChannelSearchManagerImpl.cpp | 20 ++----- src/remoteClient/clientContextImpl.cpp | 57 +++++++++---------- 3 files changed, 34 insertions(+), 52 deletions(-) diff --git a/src/remote/pv/simpleChannelSearchManagerImpl.h b/src/remote/pv/simpleChannelSearchManagerImpl.h index a8fc32b..6a54818 100644 --- a/src/remote/pv/simpleChannelSearchManagerImpl.h +++ b/src/remote/pv/simpleChannelSearchManagerImpl.h @@ -59,11 +59,6 @@ class SimpleChannelSearchManagerImpl : public: POINTER_DEFINITIONS(SimpleChannelSearchManagerImpl); - /** - * Constructor. - * @param context - */ - static shared_pointer create(Context::shared_pointer const & context); /** * Constructor. * @param context @@ -109,8 +104,6 @@ public: /// Timer stooped callback. void timerStopped(); -private: - /** * Private constructor. * @param context @@ -118,6 +111,8 @@ private: SimpleChannelSearchManagerImpl(Context::shared_pointer const & context); void activate(); +private: + bool generateSearchRequestMessage(SearchInstance::shared_pointer const & channel, bool allowNewFrame, bool flush); static bool generateSearchRequestMessage(SearchInstance::shared_pointer const & channel, diff --git a/src/remote/simpleChannelSearchManagerImpl.cpp b/src/remote/simpleChannelSearchManagerImpl.cpp index 587859e..638f3b4 100644 --- a/src/remote/simpleChannelSearchManagerImpl.cpp +++ b/src/remote/simpleChannelSearchManagerImpl.cpp @@ -41,17 +41,9 @@ const int SimpleChannelSearchManagerImpl::MAX_FRAMES_AT_ONCE = 10; const int SimpleChannelSearchManagerImpl::DELAY_BETWEEN_FRAMES_MS = 50; -SimpleChannelSearchManagerImpl::shared_pointer -SimpleChannelSearchManagerImpl::create(Context::shared_pointer const & context) -{ - SimpleChannelSearchManagerImpl::shared_pointer thisPtr(new SimpleChannelSearchManagerImpl(context)); - thisPtr->activate(); - return thisPtr; -} - SimpleChannelSearchManagerImpl::SimpleChannelSearchManagerImpl(Context::shared_pointer const & context) : m_context(context), - m_responseAddress(context->getSearchTransport()->getRemoteAddress()), + m_responseAddress(), // initialized in activate() m_canceled(), m_sequenceNumber(0), m_sendBuffer(MAX_UDP_UNFRAGMENTED_SEND), @@ -62,17 +54,17 @@ SimpleChannelSearchManagerImpl::SimpleChannelSearchManagerImpl(Context::shared_p m_userValueMutex(), m_mutex() { - - // initialize send buffer - initializeSendBuffer(); - - // initialize random seed with some random value srand ( time(NULL) ); } void SimpleChannelSearchManagerImpl::activate() { + m_responseAddress = Context::shared_pointer(m_context)->getSearchTransport()->getRemoteAddress(); + + // initialize send buffer + initializeSendBuffer(); + // add some jitter so that all the clients do not send at the same time double period = ATOMIC_PERIOD + (rand() % (2*PERIOD_JITTER_MS+1) - PERIOD_JITTER_MS)/(double)1000; diff --git a/src/remoteClient/clientContextImpl.cpp b/src/remoteClient/clientContextImpl.cpp index 6fe604d..473b12e 100644 --- a/src/remoteClient/clientContextImpl.cpp +++ b/src/remoteClient/clientContextImpl.cpp @@ -4145,54 +4145,49 @@ private: osiSockAttach(); m_timer.reset(new Timer("pvAccess-client timer", lowPriority)); - InternalClientContextImpl::shared_pointer thisPointer = internal_from_this(); + InternalClientContextImpl::shared_pointer thisPointer(internal_from_this()); // stores weak_ptr m_connector.reset(new BlockingTCPConnector(thisPointer, m_receiveBufferSize, m_connectionTimeout)); // stores many weak_ptr m_responseHandler.reset(new ClientResponseHandler(thisPointer)); + m_channelSearchManager.reset(new SimpleChannelSearchManagerImpl(thisPointer)); + // preinitialize security plugins SecurityPluginRegistry::instance(); // TODO put memory barrier here... (if not already called within a lock?) // setup UDP transport - initializeUDPTransport(); + { + + // query broadcast addresses of all IFs + SOCKET socket = epicsSocketCreate(AF_INET, SOCK_DGRAM, 0); + if (socket == INVALID_SOCKET) + { + throw std::logic_error("Failed to create a socket to introspect network interfaces."); + } + + IfaceNodeVector ifaceList; + if (discoverInterfaces(ifaceList, socket, 0) || ifaceList.size() == 0) + { + LOG(logLevelError, "Failed to introspect interfaces or no network interfaces available."); + } + epicsSocketDestroy (socket); + + initializeUDPTransports(false, m_udpTransports, ifaceList, m_responseHandler, m_searchTransport, + m_broadcastPort, m_autoAddressList, m_addressList, std::string()); + + } // setup search manager - m_channelSearchManager = SimpleChannelSearchManagerImpl::create(thisPointer); + // Starts timer + m_channelSearchManager->activate(); // TODO what if initialization failed!!! } - /** - * Initialized UDP transport (broadcast socket and repeater connection). - */ - bool initializeUDPTransport() { - - // query broadcast addresses of all IFs - SOCKET socket = epicsSocketCreate(AF_INET, SOCK_DGRAM, 0); - if (socket == INVALID_SOCKET) - { - LOG(logLevelError, "Failed to create a socket to introspect network interfaces."); - return false; - } - - IfaceNodeVector ifaceList; - if (discoverInterfaces(ifaceList, socket, 0) || ifaceList.size() == 0) - { - LOG(logLevelError, "Failed to introspect interfaces or no network interfaces available."); - return false; - } - epicsSocketDestroy (socket); - - initializeUDPTransports(false, m_udpTransports, ifaceList, m_responseHandler, m_searchTransport, - m_broadcastPort, m_autoAddressList, m_addressList, std::string()); - - return true; - } - void internalDestroy() { // @@ -4591,7 +4586,7 @@ private: * Channel search manager. * Manages UDP search requests. */ - ChannelSearchManager::shared_pointer m_channelSearchManager; + SimpleChannelSearchManagerImpl::shared_pointer m_channelSearchManager; /** * Beacon handler map.