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
