From 45a99f08301190f7a493c26ff60c5e4011802245 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Wed, 15 Feb 2023 17:33:31 +0000 Subject: [PATCH] HPCC-28920 Roxie crash in DBGLOG from deblacklist() Additional fix was needed. Signed-off-by: Richard Chapman --- common/thorhelper/thorsoapcall.cpp | 1 + roxie/ccd/ccdmain.cpp | 2 +- system/jlib/jlog.cpp | 18 ++++++---------- system/jlib/jthread.cpp | 33 ++++++++++++++++++++++-------- system/jlib/jthread.hpp | 9 +++++--- system/jlib/jtrace.hpp | 13 +++++------- 6 files changed, 44 insertions(+), 32 deletions(-) diff --git a/common/thorhelper/thorsoapcall.cpp b/common/thorhelper/thorsoapcall.cpp index dfee0cd7d51..23120fa4157 100644 --- a/common/thorhelper/thorsoapcall.cpp +++ b/common/thorhelper/thorsoapcall.cpp @@ -464,6 +464,7 @@ class BlackLister : public CInterface, implements IThreadFactory unsigned delay = 5000; for (unsigned i = 0; i < BLACKLIST_RETRIES; i++) { + // DBGLOG("Trying to deblacklist"); try { Owned s = ISocket::connect_timeout(ep, 10000); diff --git a/roxie/ccd/ccdmain.cpp b/roxie/ccd/ccdmain.cpp index 698570d3b2e..42fd484785c 100644 --- a/roxie/ccd/ccdmain.cpp +++ b/roxie/ccd/ccdmain.cpp @@ -828,7 +828,7 @@ int CCD_API roxie_main(int argc, const char *argv[], const char * defaultYaml) // Tracing feature flags TraceFlags traceLevelFlag = traceLevel ? TraceFlags::Standard : TraceFlags::None; - updateTraceFlags(loadTraceFlags(topology, roxieTraceOptions, traceLevelFlag | traceRoxieActiveQueries)); + updateTraceFlags(loadTraceFlags(topology, roxieTraceOptions, traceLevelFlag | traceRoxieActiveQueries), true); //Logging stuff #ifndef _CONTAINERIZED diff --git a/system/jlib/jlog.cpp b/system/jlib/jlog.cpp index 201703df57a..d775c556d57 100644 --- a/system/jlib/jlog.cpp +++ b/system/jlib/jlog.cpp @@ -261,6 +261,7 @@ static LogMsgJobInfo globalDefaultJobInfo(UnknownJob, UnknownUser); // NOTE - extern thread_local variables are very inefficient - don't be tempted to expose the variables directly +static TraceFlags defaultTraceFlags = TraceFlags::Standard; static thread_local LogMsgJobInfo defaultJobInfo; static thread_local TraceFlags threadTraceFlags = TraceFlags::Standard; static thread_local const IContextLogger *default_thread_logctx = nullptr; @@ -3130,16 +3131,10 @@ bool doTrace(TraceFlags featureFlag, TraceFlags level) return (threadTraceFlags & featureFlag) == featureFlag; } -void setTraceFlag(TraceFlags flag, bool enable) -{ - if (enable) - threadTraceFlags |= flag; - else - threadTraceFlags &= ~flag; -} - -void updateTraceFlags(TraceFlags flag) +void updateTraceFlags(TraceFlags flag, bool global) { + if (global) + defaultTraceFlags = flag; threadTraceFlags = flag; } @@ -3148,10 +3143,9 @@ TraceFlags queryTraceFlags() return threadTraceFlags; } -void setTraceLevel(TraceFlags level) +TraceFlags queryDefaultTraceFlags() { - threadTraceFlags &= ~TraceFlags::LevelMask; - threadTraceFlags |= (level & TraceFlags::LevelMask); + return defaultTraceFlags; } LogContextScope::LogContextScope(const IContextLogger *ctx) diff --git a/system/jlib/jthread.cpp b/system/jlib/jthread.cpp index 9f8d9eb1d49..c41c92898fe 100644 --- a/system/jlib/jthread.cpp +++ b/system/jlib/jthread.cpp @@ -371,6 +371,17 @@ void Thread::init(const char *_name) stacksize = 0; // default is EXE default stack size (set by /STACK) } +void Thread::getThreadLoggingInfo() +{ + ::getThreadLoggingInfo(logctx, traceFlags); +} + +void Thread::setThreadLoggingInfo(const IContextLogger *_logctx, TraceFlags _traceFlags) +{ + logctx = _logctx; + traceFlags = _traceFlags; +} + void Thread::start() { if (alive) { @@ -382,7 +393,6 @@ void Thread::start() return; } Link(); - getThreadLoggingInfo(logctx, traceFlags); // New thread uses context from parent. This may or may not be a good idea by default! startRelease(); } @@ -562,7 +572,7 @@ void CThreadedPersistent::threadmain() break; try { - resetThreadLogging(logctx, traceFlags); + resetThreadLogging(athread.logctx, athread.traceFlags); owner->threadmain(); // Note we do NOT call the thread reset hook here - these threads are expected to be able to preserve state, I think } @@ -594,7 +604,6 @@ void CThreadedPersistent::start() PrintStackReport(); throw MakeStringExceptionDirect(-1, msg.str()); } - getThreadLoggingInfo(logctx, traceFlags); // New thread uses context from parent. This may or may not be a good idea by default! sem.signal(); } @@ -674,6 +683,7 @@ void CAsyncFor::For(unsigned num,unsigned maxatonce,bool abortFollowingException { idx = _idx; self = _self; + getThreadLoggingInfo(); } int run() { @@ -726,6 +736,7 @@ void CAsyncFor::For(unsigned num,unsigned maxatonce,bool abortFollowingException { idx = _idx; self = _self; + getThreadLoggingInfo(); } int run() { @@ -805,10 +816,7 @@ class CPooledThreadWrapper; class CThreadPoolBase { public: - CThreadPoolBase() - { - getThreadLoggingInfo(logctx, traceFlags); // Threads in pool use context that was in force when threadpool was created. This may or may not be a good idea by default! - } + CThreadPoolBase() {} virtual ~CThreadPoolBase() {} protected: friend class CPooledThreadWrapper; IExceptionHandler *exceptionHandler; @@ -823,7 +831,7 @@ protected: friend class CPooledThreadWrapper; unsigned targetpoolsize; unsigned delay; const IContextLogger *logctx = nullptr; - TraceFlags traceFlags = TraceFlags::Standard; + TraceFlags traceFlags = queryDefaultTraceFlags(); Semaphore availsem; std::atomic_uint numrunning{0}; virtual void notifyStarted(CPooledThreadWrapper *item)=0; @@ -1277,6 +1285,15 @@ class CThreadPool: public CThreadPoolBase, implements IThreadPool, public CInter } return false; } + void getThreadLoggingInfo() + { + ::getThreadLoggingInfo(logctx, traceFlags); + } + void setThreadLoggingInfo(const IContextLogger *_logctx, TraceFlags _traceFlags) + { + logctx = _logctx; + traceFlags = _traceFlags; + } }; diff --git a/system/jlib/jthread.hpp b/system/jlib/jthread.hpp index fc158698986..922276b1ed1 100644 --- a/system/jlib/jthread.hpp +++ b/system/jlib/jthread.hpp @@ -76,6 +76,7 @@ class jlib_decl QueryTerminationCleanup class jlib_decl Thread : public CInterface, public IThread { +friend class CThreadedPersistent; private: ThreadId threadid; unsigned short stacksize; // in 4K blocks @@ -98,7 +99,7 @@ class jlib_decl Thread : public CInterface, public IThread protected: StringAttr cthreadname; const IContextLogger *logctx = nullptr; - TraceFlags traceFlags = TraceFlags::Standard; + TraceFlags traceFlags = queryDefaultTraceFlags(); public: #ifndef _WIN32 Semaphore suspend; @@ -119,6 +120,8 @@ class jlib_decl Thread : public CInterface, public IThread const char *getName() { return cthreadname.isEmpty() ? "unknown" : cthreadname.str(); } bool isAlive() { return alive; } bool join(unsigned timeout=INFINITE); + void getThreadLoggingInfo(); // Capture current thread logging context to be used by this thread when started + void setThreadLoggingInfo(const IContextLogger * _logctx, TraceFlags _traceFlags); // Set a specified thread logging context to be used when this thread is started virtual void start(); virtual void startRelease(); @@ -174,8 +177,6 @@ class jlib_decl CThreadedPersistent std::atomic_uint state; bool halt; enum ThreadStates { s_ready, s_running, s_joining }; - const IContextLogger *logctx = nullptr; - TraceFlags traceFlags = TraceFlags::Standard; void threadmain(); public: CThreadedPersistent(const char *name, IThreaded *_owner); @@ -267,6 +268,8 @@ interface IThreadPool : extends IInterface virtual PooledThreadHandle startNoBlock(void *param)=0; // starts a new thread if it can do so without blocking, else throws exception virtual void setStartDelayTracing(unsigned secs) = 0; // set start delay tracing period virtual bool waitAvailable(unsigned timeout) = 0; // wait until a pool member is available + virtual void getThreadLoggingInfo() = 0; // Capture current thread logging context to be used by thread in pool when started + virtual void setThreadLoggingInfo(const IContextLogger * _logctx, TraceFlags _traceFlags) = 0; // Set a specified thread logging context to be used by thredas in pool when started }; extern jlib_decl IThreadPool *createThreadPool( diff --git a/system/jlib/jtrace.hpp b/system/jlib/jtrace.hpp index 8d198b52896..4781bdf172e 100644 --- a/system/jlib/jtrace.hpp +++ b/system/jlib/jtrace.hpp @@ -114,18 +114,15 @@ interface IPropertyTree; extern jlib_decl bool doTrace(TraceFlags featureFlag, TraceFlags level=TraceFlags::Standard); -// Overwrites current trace flags for active thread (and any it creates) -extern jlib_decl void updateTraceFlags(TraceFlags flag); - -// Set a single trace flags for the active thread (and any it creates) -extern jlib_decl void setTraceFlag(TraceFlags flag, bool enable); - -// Set the trace detail level for the active thread (and any it creates) -extern jlib_decl void setTraceLevel(TraceFlags level); +// Overwrites current trace flags for active thread (and optionally the global default for new threads) +extern jlib_decl void updateTraceFlags(TraceFlags flag, bool global = false); // Retrieve current trace flags for the active thread extern jlib_decl TraceFlags queryTraceFlags(); +// Retrieve default trace flags for new threads +extern jlib_decl TraceFlags queryDefaultTraceFlags(); + // Load trace flags from a property tree - typically the global config // See also the workunit-variant in workunit.hpp