1.Renamed name to fname for removeTemporaryFile.

2.Replaced tempOwnerId with tempFilePrefix which includes wuid.
3.tempFilePrefix is set in EclAgent constructor.
4.Removed inline conversion to std::string() as unnecessary.
5.Ensure only tempFileSet functionality is executed after tfsect.
6.Added missing overrides.
7.Removed mutable from CriticalSection tfsect as unnecessary.

Signed-off-by: Dave Streeter <dave.streeter@lexisnexisrisk.com>

Author: streeterd

ecl/eclagent/agentctx.hpp

@@ -82,9 +82,12 @@ enum class AccessMode : unsigned;
 
 interface ITempFileHandler
 {
+    // name is the name/id of the temporary file, not the filename on disk.
     virtual const char *noteTemporaryFile(const char *name) = 0;
+    // name is the name/id of the temporary file, not the filename on disk.
     virtual const char *queryTemporaryFile(const char *name) = 0;
-    virtual void removeTemporaryFile(const char *name) = 0;
+    // fname is the temporary filename on disk returned from noteTemporaryFile or queryTemporaryFile.
+    virtual void removeTemporaryFile(const char *fname) = 0;
 };
 
 struct IAgentContext : extends IGlobalCodeContext, extends ITempFileHandler

ecl/eclagent/eclagent.cpp

@@ -565,6 +565,9 @@ EclAgent::EclAgent(IConstWorkUnit *wu, const char *_wuid, bool _checkVersion, bo
     abortmonitor->setGuillotineCost(getGuillotineCost(wu));
     configurePreferredPlanes();
     agentMachineCost = getMachineCostRate();
+
+    getTempfileBase(tempFilePrefix);
+    tempFilePrefix.append(PATHSEPCHAR);
 }
 
 EclAgent::~EclAgent()
@@ -610,41 +613,30 @@ const char *EclAgent::queryTempfilePath()
     return agentTempDir.str();
 }
 
-StringBuffer & EclAgent::getTempfileBase(StringBuffer & buff)
+StringBuffer &EclAgent::getTempfileBase(StringBuffer &buff)
 {
     return buff.append(queryTempfilePath()).append(PATHSEPCHAR).appendLower(wuid);
 }
 
 void EclAgent::buildTempFilename(StringBuffer &tempFilename, const char *name)
 {
-    /* tempOwnerId is used at the end of the temporary file name to ensure
-     * that in the event of a crash, we can use the pid and start time to
-     * determine if temp files are from a currently running process or not,
-     * and therefore whether they can be safely deleted.
-     */
-    if (tempOwnerId.isEmpty())
-    {
-        unsigned pid = (unsigned)GetCurrentProcessId();
-        unsigned __int64 startEpoch = (unsigned __int64)time(nullptr);
-        tempOwnerId.append('.').append(pid).append('.').append(startEpoch);
-    }
-
-    getTempfileBase(tempFilename);
-    tempFilename.append(PATHSEPCHAR).append('.').append(name).append(tempOwnerId);
+    tempFilename.append(tempFilePrefix).append(name);
 }
 
 const char *EclAgent::queryTemporaryFile(const char *name)
 {
     dbgassertex(!isEmptyString(name));
 
-    CriticalBlock crit(tfsect);
-
     StringBuffer tempFilename;
     buildTempFilename(tempFilename, name);
 
-    auto it = tempFileSet.find(std::string(tempFilename.str()));
-    if (it != tempFileSet.end())
-        return it->c_str();
+    {
+        CriticalBlock crit(tfsect);
+
+        auto it = tempFileSet.find(tempFilename.str());
+        if (it != tempFileSet.end())
+            return it->c_str();
+    }
 
     VStringBuffer errmsg("Attempt to read temp file that has not yet been registered: %s", name);
     fail(0, errmsg.str());
@@ -655,12 +647,16 @@ const char *EclAgent::noteTemporaryFile(const char *name)
 {
     dbgassertex(!isEmptyString(name));
 
-    CriticalBlock crit(tfsect);
-
     StringBuffer tempFilename;
     buildTempFilename(tempFilename, name);
 
-    auto inserted = tempFileSet.emplace(tempFilename.str());
+    std::pair<std::set<std::string>::iterator, bool> inserted;
+    {
+        CriticalBlock crit(tfsect);
+
+        inserted = tempFileSet.emplace(tempFilename.str());
+    }
+
     if (!inserted.second)
     {
         VStringBuffer errmsg("Temp file already registered: %s", name);
@@ -671,13 +667,13 @@ const char *EclAgent::noteTemporaryFile(const char *name)
     return inserted.first->c_str();
 }
 
-void EclAgent::removeTemporaryFile(const char *name)
+void EclAgent::removeTemporaryFile(const char *fname)
 {
-    dbgassertex(!isEmptyString(name));
+    dbgassertex(!isEmptyString(fname));
 
     CriticalBlock crit(tfsect);
 
-    auto it = tempFileSet.find(std::string(name));
+    auto it = tempFileSet.find(std::string(fname));
     dbgassertex(it != tempFileSet.end());
     if (it != tempFileSet.end())
     {
@@ -690,9 +686,8 @@ void EclAgent::deleteTempFiles()
 {
     CriticalBlock crit(tfsect);
 
-    if (!tempOwnerId.isEmpty())
-        while (!tempFileSet.empty())
-            removeTemporaryFile(tempFileSet.begin()->c_str());
+    while (!tempFileSet.empty())
+        removeTemporaryFile(tempFileSet.begin()->c_str());
 }
 
 const char *EclAgent::loadResource(unsigned id)

ecl/eclagent/eclagent.ipp

@@ -174,17 +174,17 @@ public:
     {
         return ctx->getTempfileBase(buff);
     }
-    virtual const char *noteTemporaryFile(const char *name)
+    virtual const char *noteTemporaryFile(const char *name) override
     {
         return ctx->noteTemporaryFile(name);
     }
-    virtual const char *queryTemporaryFile(const char *name)
+    virtual const char *queryTemporaryFile(const char *name) override
     {
         return ctx->queryTemporaryFile(name);
     }
-    virtual void removeTemporaryFile(const char *name)
+    virtual void removeTemporaryFile(const char *fname) override
     {
-        ctx->removeTemporaryFile(name);
+        ctx->removeTemporaryFile(fname);
     }
     virtual void reloadWorkUnit()
     {
@@ -380,8 +380,8 @@ private:
     unsigned __int64 stopAfter;
     mutable CriticalSection wusect;
     std::set<std::string> tempFileSet; // Set of actual temp file names on disk
-    StringBuffer tempOwnerId; // Holds the current pid and start epoch of the process for temp files in map tempFileNameById
-    mutable CriticalSection tfsect;
+    StringBuffer tempFilePrefix; // Spill and WUID folder path for temp files
+    CriticalSection tfsect;
     IArray persistReadLocks;
     StringArray processedPersists;
 
@@ -618,9 +618,9 @@ public:
 
     virtual unsigned __int64 getDatasetHash(const char * name, unsigned __int64 hash);
     virtual void reportProgress(const char *msg, unsigned flags=0);
-    virtual const char *noteTemporaryFile(const char *name);
-    virtual const char *queryTemporaryFile(const char *name);
-    virtual void removeTemporaryFile(const char *name);
+    virtual const char *noteTemporaryFile(const char *name) override;
+    virtual const char *queryTemporaryFile(const char *name) override;
+    virtual void removeTemporaryFile(const char *fname) override;
     virtual void deleteFile(const char * logicalName);
 
     void addException(ErrorSeverity severity, const char * source, unsigned code, const char * text, const char * filename, unsigned lineno, unsigned column, bool failOnError, bool isAbort);