Merge pull request #16991 from ghalliday/deblacklist-2

HPCC-28920 Roxie crash in DBGLOG from deblacklist() Reviewed-by: Mark Kelly [email protected] Reviewed-by: Gavin Halliday <[email protected]> Merged-by: Gavin Halliday <[email protected]>
hpcc-systems · Feb 22, 2023 · a2b2939 · a2b2939
2 parents aaf6a28 + 45a99f0
commit a2b2939
Show file tree

Hide file tree

Showing 6 changed files with 44 additions and 32 deletions.
diff --git 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<ISocket> s = ISocket::connect_timeout(ep, 10000);

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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