Hi, Have you had time to review this ticket? Thanks.
Regards, Vu > -----Original Message----- > From: Vu Minh Nguyen <vu.m.ngu...@dektech.com.au> > Sent: Thursday, January 24, 2019 5:15 PM > To: lennart.l...@ericsson.com; canh.v.tru...@dektech.com.au > Cc: opensaf-devel@lists.sourceforge.net; Vu Minh Nguyen > <vu.m.ngu...@dektech.com.au> > Subject: [PATCH 1/1] log: fix coredump at log agent application [#3002] > > There is a race in using singleton-static class object b/w mds thread > and application thread - caller of exit() api. > > This patch still uses singleton but making the instance shared_ptr > to ensure the resource will not be destroyed if it is being used. > --- > src/log/agent/lga_agent.cc | 7 ++----- > src/log/agent/lga_agent.h | 21 ++++++++++++++------- > src/log/agent/lga_api.cc | 20 ++++++++++---------- > src/log/agent/lga_mds.cc | 32 ++++++++++++++++---------------- > src/log/agent/lga_state.cc | 2 +- > src/log/agent/lga_util.cc | 10 +++++----- > 6 files changed, 48 insertions(+), 44 deletions(-) > > diff --git a/src/log/agent/lga_agent.cc b/src/log/agent/lga_agent.cc > index bf9caa935..1000bb3fd 100644 > --- a/src/log/agent/lga_agent.cc > +++ b/src/log/agent/lga_agent.cc > @@ -108,7 +108,7 @@ ScopeData::~ScopeData() { > recovery2_unlock(is_locked_); > } > > - LogAgent::instance().EnterCriticalSection(); > + LogAgent::instance()->EnterCriticalSection(); > LogClient* client = client_data_->client; > bool* is_updated = client_data_->is_updated; > RefCounter::Degree client_degree = client_data_->value; > @@ -128,15 +128,12 @@ ScopeData::~ScopeData() { > stream->RestoreRefCounter(caller, stream_degree, > *stream_is_updated); > } > } > - LogAgent::instance().LeaveCriticalSection(); > + LogAgent::instance()->LeaveCriticalSection(); > } > > //-------------------------------------------------------------------------- ---- > // LogAgent > //-------------------------------------------------------------------------- ---- > -// Singleton represents LOG agent. > -LogAgent LogAgent::me_; > - > LogAgent::LogAgent() { > client_list_.clear(); > // There is high risk of calling one @LogClient method > diff --git a/src/log/agent/lga_agent.h b/src/log/agent/lga_agent.h > index 0049da054..0c32ea33b 100644 > --- a/src/log/agent/lga_agent.h > +++ b/src/log/agent/lga_agent.h > @@ -19,12 +19,14 @@ > #define SRC_LOG_AGENT_LGA_AGENT_H_ > > #include <atomic> > +#include <memory> > #include <string> > #include <vector> > -#include "mds/mds_papi.h" > #include <saAis.h> > -#include "base/macros.h" > #include <saLog.h> > + > +#include "base/macros.h" > +#include "mds/mds_papi.h" > #include "log/common/lgsv_msg.h" > #include "log/common/lgsv_defs.h" > #include "log/agent/lga_common.h" > @@ -80,7 +82,15 @@ class LogClient; > //< > class LogAgent { > public: > - static LogAgent& instance() { return me_; } > + static std::shared_ptr<LogAgent>& instance() { > + // Ensure this static singleton instance is only destroyed when > + // no one is using it. Note that: static data can be destroyed > + // in log application thread which calls exit() libc. So, introducing > + // shared_ptr<> to avoid races among threads. > + static std::shared_ptr<LogAgent> me = > + std::shared_ptr<LogAgent>{new LogAgent()}; > + return me; > + } > > //< > // C++ APIs wrapper for corresponding C LOG Agent APIs > @@ -158,11 +168,11 @@ class LogAgent { > // Introduce these public interface for MDS thread use. > void EnterCriticalSection(); > void LeaveCriticalSection(); > + ~LogAgent() {} > > private: > // Not allow to create @LogAgent object, except the singleton object > @me_. > LogAgent(); > - ~LogAgent() {} > > // True if there is no Active SC, otherwise false > bool no_active_log_server() const; > @@ -274,9 +284,6 @@ class LogAgent { > // LGS LGA sync params > NCS_SEL_OBJ lgs_sync_sel_; > > - // Singleton represents LOG Agent in LOG application process > - static LogAgent me_; > - > DELETE_COPY_AND_MOVE_OPERATORS(LogAgent); > }; > > diff --git a/src/log/agent/lga_api.cc b/src/log/agent/lga_api.cc > index 37a1cc650..97dbf1d31 100644 > --- a/src/log/agent/lga_api.cc > +++ b/src/log/agent/lga_api.cc > @@ -39,7 +39,7 @@ > SaAisErrorT saLogInitialize(SaLogHandleT* logHandle, > const SaLogCallbacksT* callbacks, > SaVersionT* version) { > - return LogAgent::instance().saLogInitialize(logHandle, callbacks, version); > + return LogAgent::instance()->saLogInitialize(logHandle, callbacks, version); > } > > > /************************************************************** > ************* > @@ -69,7 +69,7 @@ SaAisErrorT saLogInitialize(SaLogHandleT* logHandle, > > ************************************************************** > *************/ > SaAisErrorT saLogSelectionObjectGet(SaLogHandleT logHandle, > SaSelectionObjectT* selectionObject) { > - return LogAgent::instance().saLogSelectionObjectGet(logHandle, > + return LogAgent::instance()->saLogSelectionObjectGet(logHandle, > selectionObject); > } > > @@ -96,7 +96,7 @@ SaAisErrorT saLogSelectionObjectGet(SaLogHandleT > logHandle, > > ************************************************************** > *************/ > SaAisErrorT saLogDispatch(SaLogHandleT logHandle, > SaDispatchFlagsT dispatchFlags) { > - return LogAgent::instance().saLogDispatch(logHandle, dispatchFlags); > + return LogAgent::instance()->saLogDispatch(logHandle, dispatchFlags); > } > > > /************************************************************** > ************* > @@ -122,7 +122,7 @@ SaAisErrorT saLogDispatch(SaLogHandleT logHandle, > * > > ************************************************************** > *************/ > SaAisErrorT saLogFinalize(SaLogHandleT logHandle) { > - return LogAgent::instance().saLogFinalize(logHandle); > + return LogAgent::instance()->saLogFinalize(logHandle); > } > > /** > @@ -142,7 +142,7 @@ SaAisErrorT saLogStreamOpen_2( > const SaLogFileCreateAttributesT_2* logFileCreateAttributes, > SaLogStreamOpenFlagsT logStreamOpenFlags, SaTimeT timeOut, > SaLogStreamHandleT* logStreamHandle) { > - return LogAgent::instance().saLogStreamOpen_2( > + return LogAgent::instance()->saLogStreamOpen_2( > logHandle, logStreamName, logFileCreateAttributes, > logStreamOpenFlags, > timeOut, logStreamHandle); > } > @@ -161,7 +161,7 @@ SaAisErrorT saLogStreamOpenAsync_2( > SaLogHandleT logHandle, const SaNameT* logStreamName, > const SaLogFileCreateAttributesT_2* logFileCreateAttributes, > SaLogStreamOpenFlagsT logstreamOpenFlags, SaInvocationT invocation) { > - return LogAgent::instance().saLogStreamOpenAsync_2( > + return LogAgent::instance()->saLogStreamOpenAsync_2( > logHandle, logStreamName, logFileCreateAttributes, > logstreamOpenFlags, > invocation); > } > @@ -176,7 +176,7 @@ SaAisErrorT saLogStreamOpenAsync_2( > */ > SaAisErrorT saLogWriteLog(SaLogStreamHandleT logStreamHandle, SaTimeT > timeOut, > const SaLogRecordT* logRecord) { > - return LogAgent::instance().saLogWriteLog(logStreamHandle, timeOut, > + return LogAgent::instance()->saLogWriteLog(logStreamHandle, timeOut, > logRecord); > } > > @@ -193,7 +193,7 @@ SaAisErrorT > saLogWriteLogAsync(SaLogStreamHandleT logStreamHandle, > SaInvocationT invocation, > SaLogAckFlagsT ackFlags, > const SaLogRecordT* logRecord) { > - return LogAgent::instance().saLogWriteLogAsync(logStreamHandle, > invocation, > + return LogAgent::instance()->saLogWriteLogAsync(logStreamHandle, > invocation, > ackFlags, logRecord); > } > /** > @@ -204,7 +204,7 @@ SaAisErrorT > saLogWriteLogAsync(SaLogStreamHandleT logStreamHandle, > * @return SaAisErrorT > */ > SaAisErrorT saLogStreamClose(SaLogStreamHandleT logStreamHandle) { > - return LogAgent::instance().saLogStreamClose(logStreamHandle); > + return LogAgent::instance()->saLogStreamClose(logStreamHandle); > } > > /** > @@ -217,5 +217,5 @@ SaAisErrorT saLogStreamClose(SaLogStreamHandleT > logStreamHandle) { > */ > SaAisErrorT saLogLimitGet(SaLogHandleT logHandle, SaLogLimitIdT limitId, > SaLimitValueT* limitValue) { > - return LogAgent::instance().saLogLimitGet(logHandle, limitId, limitValue); > + return LogAgent::instance()->saLogLimitGet(logHandle, limitId, limitValue); > } > diff --git a/src/log/agent/lga_mds.cc b/src/log/agent/lga_mds.cc > index ca949577f..07db48d2c 100644 > --- a/src/log/agent/lga_mds.cc > +++ b/src/log/agent/lga_mds.cc > @@ -516,9 +516,9 @@ static uint32_t lga_lgs_msg_proc(lgsv_msg_t > *lgsv_msg, > // Lookup the hdl rec by client_id > LogClient *client = nullptr; > uint32_t id = lgsv_msg->info.cbk_info.lgs_client_id; > - LogAgent::instance().EnterCriticalSection(); > - if (nullptr == (client = LogAgent::instance().SearchClientById(id))) { > - LogAgent::instance().LeaveCriticalSection(); > + LogAgent::instance()->EnterCriticalSection(); > + if (nullptr == (client = LogAgent::instance()->SearchClientById(id))) { > + LogAgent::instance()->LeaveCriticalSection(); > TRACE("regid not found"); > lga_msg_destroy(lgsv_msg); > return NCSCC_RC_FAILURE; > @@ -527,12 +527,12 @@ static uint32_t lga_lgs_msg_proc(lgsv_msg_t > *lgsv_msg, > // @client is being deleted in other thread. DO NOT touch this. > bool updated = false; > if (client->FetchAndIncreaseRefCounter(__func__, &updated) == -1) { > - LogAgent::instance().LeaveCriticalSection(); > + LogAgent::instance()->LeaveCriticalSection(); > lga_msg_destroy(lgsv_msg); > TRACE_LEAVE(); > return rc; > } > - LogAgent::instance().LeaveCriticalSection(); > + LogAgent::instance()->LeaveCriticalSection(); > > switch (lgsv_msg->type) { > case LGSV_LGS_CBK_MSG: > @@ -554,7 +554,7 @@ static uint32_t lga_lgs_msg_proc(lgsv_msg_t > *lgsv_msg, > > TRACE_2("LGSV_CLM_NODE_STATUS_CALLBACK clm_node_status: > %d", status); > std::atomic<SaClmClusterChangesT> &clm_node_state = > - LogAgent::instance().atomic_get_clm_node_state(); > + LogAgent::instance()->atomic_get_clm_node_state(); > clm_node_state = > lgsv_msg->info.cbk_info.clm_node_status_cbk.clm_node_status; > // A client becomes stale if Node loses CLM Membership. > @@ -644,7 +644,7 @@ static uint32_t lga_mds_svc_evt(struct > ncsmds_callback_info *mds_cb_info) { > TRACE("%s\t NCSMDS_NO_ACTIVE", __func__); > // This is a temporary server down e.g. during switch/fail over > if (mds_cb_info->info.svc_evt.i_svc_id == NCSMDS_SVC_ID_LGS) { > - LogAgent::instance().NoActiveLogServer(); > + LogAgent::instance()->NoActiveLogServer(); > } > break; > > @@ -658,7 +658,7 @@ static uint32_t lga_mds_svc_evt(struct > ncsmds_callback_info *mds_cb_info) { > // Notify to LOG Agent that no LOG server exist. > // In turn, it will inform to all its log clients > // and all log streams belong to each log client. > - LogAgent::instance().NoLogServer(); > + LogAgent::instance()->NoLogServer(); > // Stop the recovery thread if it is running > lga_no_server_state_set(); > } > @@ -671,11 +671,11 @@ static uint32_t lga_mds_svc_evt(struct > ncsmds_callback_info *mds_cb_info) { > TRACE("%s\t NCSMDS_UP", __func__); > // Inform to LOG agent that LOG server is up from headless > // and provide it the LOG server destination address too. > - LogAgent::instance().HasActiveLogServer( > + LogAgent::instance()->HasActiveLogServer( > mds_cb_info->info.svc_evt.i_dest); > - if (LogAgent::instance().waiting_log_server_up() == true) { > + if (LogAgent::instance()->waiting_log_server_up() == true) { > // Signal waiting thread > - m_NCS_SEL_OBJ_IND(LogAgent::instance().get_lgs_sync_sel()); > + m_NCS_SEL_OBJ_IND(LogAgent::instance()->get_lgs_sync_sel()); > } > // Start recovery > lga_serv_recov1state_set(); > @@ -1236,7 +1236,7 @@ uint32_t lga_mds_init() { > NCSMDS_INFO mds_info; > uint32_t rc = NCSCC_RC_SUCCESS; > MDS_SVC_ID svc = NCSMDS_SVC_ID_LGS; > - std::atomic<MDS_HDL> &mds_hdl = > LogAgent::instance().atomic_get_mds_hdl(); > + std::atomic<MDS_HDL> &mds_hdl = LogAgent::instance()- > >atomic_get_mds_hdl(); > > TRACE_ENTER(); > // Create the ADEST for LGA and get the pwe hdl > @@ -1307,9 +1307,9 @@ uint32_t lga_mds_msg_sync_send(lgsv_msg_t > *i_msg, lgsv_msg_t **o_msg, > SaTimeT timeout, uint32_t prio) { > NCSMDS_INFO mds_info; > uint32_t rc = NCSCC_RC_SUCCESS; > - std::atomic<MDS_HDL> &mds_hdl = > LogAgent::instance().atomic_get_mds_hdl(); > + std::atomic<MDS_HDL> &mds_hdl = LogAgent::instance()- > >atomic_get_mds_hdl(); > std::atomic<MDS_DEST> &lgs_mds_dest = > - LogAgent::instance().atomic_get_lgs_mds_dest(); > + LogAgent::instance()->atomic_get_lgs_mds_dest(); > > TRACE_ENTER(); > > @@ -1358,9 +1358,9 @@ uint32_t lga_mds_msg_sync_send(lgsv_msg_t > *i_msg, lgsv_msg_t **o_msg, > > ************************************************************** > ****************/ > uint32_t lga_mds_msg_async_send(lgsv_msg_t *i_msg, uint32_t prio) { > NCSMDS_INFO mds_info; > - std::atomic<MDS_HDL> &mds_hdl = > LogAgent::instance().atomic_get_mds_hdl(); > + std::atomic<MDS_HDL> &mds_hdl = LogAgent::instance()- > >atomic_get_mds_hdl(); > std::atomic<MDS_DEST> &lgs_mds_dest = > - LogAgent::instance().atomic_get_lgs_mds_dest(); > + LogAgent::instance()->atomic_get_lgs_mds_dest(); > > TRACE_ENTER(); > osafassert(i_msg != nullptr); > diff --git a/src/log/agent/lga_state.cc b/src/log/agent/lga_state.cc > index 05c7a85ee..970fe27d2 100644 > --- a/src/log/agent/lga_state.cc > +++ b/src/log/agent/lga_state.cc > @@ -97,7 +97,7 @@ static void *recovery2_thread(void *dummy) { > > // Recover all log clients. During this time, any call of LOG APIs > // return TRY_AGAIN until the recovery is done. > - LogAgent::instance().RecoverAllLogClients(); > + LogAgent::instance()->RecoverAllLogClients(); > > // All clients are recovered (or removed). Or recovery is aborted > // Change to not recovering state RecoveryState::kNormal > diff --git a/src/log/agent/lga_util.cc b/src/log/agent/lga_util.cc > index f4d0f970a..0bb91aaee 100644 > --- a/src/log/agent/lga_util.cc > +++ b/src/log/agent/lga_util.cc > @@ -40,17 +40,17 @@ static unsigned int lga_create() { > unsigned int rc = NCSCC_RC_SUCCESS; > > // Create and init sel obj for mds sync > - NCS_SEL_OBJ* lgs_sync_sel = LogAgent::instance().get_lgs_sync_sel(); > + NCS_SEL_OBJ* lgs_sync_sel = LogAgent::instance()->get_lgs_sync_sel(); > m_NCS_SEL_OBJ_CREATE(lgs_sync_sel); > std::atomic<bool>& lgs_sync_wait = > - LogAgent::instance().atomic_get_lgs_sync_wait(); > + LogAgent::instance()->atomic_get_lgs_sync_wait(); > lgs_sync_wait = true; > > // register with MDS > if ((NCSCC_RC_SUCCESS != (rc = lga_mds_init()))) { > rc = NCSCC_RC_FAILURE; > // Delete the lga init instances > - LogAgent::instance().RemoveAllLogClients(); > + LogAgent::instance()->RemoveAllLogClients(); > return rc; > } > > @@ -64,7 +64,7 @@ static unsigned int lga_create() { > > lgs_sync_wait = false; > std::atomic<SaClmClusterChangesT>& clm_state = > - LogAgent::instance().atomic_get_clm_node_state(); > + LogAgent::instance()->atomic_get_clm_node_state(); > clm_state = SA_CLM_NODE_JOINED; > > // No longer needed > @@ -82,7 +82,7 @@ static unsigned int lga_create() { > unsigned int lga_startup() { > unsigned int rc = NCSCC_RC_SUCCESS; > ScopeLock lock(init_lock); > - std::atomic<MDS_HDL>& mds_hdl = > LogAgent::instance().atomic_get_mds_hdl(); > + std::atomic<MDS_HDL>& mds_hdl = LogAgent::instance()- > >atomic_get_mds_hdl(); > > TRACE_ENTER(); > > -- > 2.19.2 _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel