Change lockProperties API

lockProperties locks all properties in the connection, and not just
the Attr section, which is what was being returned. Now, the method
locks and return a boolean indicating if one needs to refresh its
data because the lock was not acquired clean. Using queryProperties
to get the properties after locking, to make it clear what's being
returned.

TODO: rename queryProperties to queryFileAttr (since that's what's
being returned), but this is a much bigger change (so better in a
separate patch), since there are other queryProperties around that
do different things.

dali/base/dadfs.cpp

@@ -980,7 +980,6 @@ class CDFAction: public CInterface
 protected:
     Linked<IDistributedFileTransaction> transaction;
     IArrayOf<IDistributedFile> lockedFiles;
-protected:
     enum ActionState {
         NONE,    // still not committed nor rolled back
         SUCCESS, // committing
@@ -1863,7 +1862,7 @@ public:
     void renameFile(IFile *file);
     unsigned getCRC();
     IPropertyTree &queryProperties();
-    IPropertyTree &lockProperties(unsigned timems);
+    bool lockProperties(unsigned timems);
     void unlockProperties();
     bool isHost(unsigned copy);
     offset_t getFileSize(bool allowphysical,bool forcephysical);
@@ -2286,21 +2285,19 @@ public:
         return !logicalName.isSet(); 
     }
 
-    IPropertyTree &lockProperties(unsigned timeoutms)
-    {
-        bool reload;
-        return lockProperties(reload,timeoutms);
-    }
-
-    // WARN: This method allows multiple access from the same instance to the same properties.
-    // MORE: Shouldn't we return boolean and use queryProperties() outside of this method?
-    IPropertyTree & lockProperties(bool &reload,unsigned timeoutms)
+    /*
+     *  Change connection to write-mode, allowing multiple writers only on the same instance.
+     *  Returns true if the lock was lost at least once before succeeding, hinting that some
+     *  resources might need reload (like sub-files list, etc).
+     *
+     *  WARN: This is not thread-safe
+     */
+    bool lockProperties(unsigned timeoutms)
     {
+        bool reload = false;
         if (timeoutms==INFINITE)
             timeoutms = defaultTimeout;
         reload = false;
-        // this is a bit of a kludge for non-transactional superfile operations and other dining philosopher problems
-        // WARN: This is not thread-safe
         if (proplockcount++==0) {
             if (conn) {
                 conn->rollback(); // changes chouldn't be done outside lock properties
@@ -2323,15 +2320,19 @@ public:
                 dfCheckRoot("lockProperties",root,conn);
             }
         }
-        return queryProperties();
+        return reload;
     }
 
-    // FIXME: This method is NOT unlocking the properties for exclusive access, just setting it
-    // to read mode on the last call. If concurrent access occur, there will be conflicts
+    /*
+     * Change connection back to read mode on the last unlock. There should never be
+     * an uneven number of locks/unlocks, since that will leave the connection with
+     * the DFS locked until the instance's destruction.
+     *
+     * WARN: This is not thread-safe
+     */
     void unlockProperties()
     {
         savePartsAttr();
-        // WARN: This is not thread-safe
         if (--proplockcount==0) {
             if (conn) {
 #ifdef TRACE_LOCKS
@@ -2474,7 +2475,8 @@ public:
 
     virtual void setECL(const char *ecl)
     {
-        IPropertyTree &p = lockProperties(defaultTimeout);
+        lockProperties(defaultTimeout);
+        IPropertyTree &p = queryProperties();
 #ifdef PACK_ECL
         p.removeProp("ECL");
         if (!ecl||!*ecl)
@@ -2512,7 +2514,8 @@ public:
         else {
             bool ret=false;
             if (conn) {
-                IPropertyTree &p = lockProperties(timems);
+                lockProperties(timems);
+                IPropertyTree &p = queryProperties();
                 CDateTime dt;
                 dt.setNow();
                 try {
@@ -2645,11 +2648,11 @@ public:
 
     virtual void setColumnMapping(const char *mapping)
     {
-        IPropertyTree &prop = lockProperties(defaultTimeout);
+        lockProperties(defaultTimeout);
         if (!mapping||!*mapping) 
-            prop.removeProp("@columnMapping");
+            queryProperties().removeProp("@columnMapping");
         else
-            prop.setProp("@columnMapping",mapping);
+            queryProperties().setProp("@columnMapping",mapping);
         unlockProperties();
     }
 
@@ -2813,14 +2816,12 @@ public:
                 }
             }
         }
-        lockProperties(defaultTimeout);         // only needed to load attr (no lock)
         shrinkFileTree(root);
         if (totalsize!=(offset_t)-1)
             queryProperties().setPropInt64("@size", totalsize);
         if (useableCheckSum)
             queryProperties().setPropInt64("@checkSum", checkSum);
         setModified();
-        unlockProperties();
 #ifdef EXTRA_LOGGING
         LOGPTREE("CDistributedFile.b root.2",root);
 #endif
@@ -3232,7 +3233,8 @@ public:
         }
         attach(_logicalname,transaction,user);
         if (prevname.length()) {
-            IPropertyTree &pt = lockProperties(defaultTimeout);
+            lockProperties(defaultTimeout);
+            IPropertyTree &pt = queryProperties();
             StringBuffer list;
             if (pt.getProp("@renamedFrom",list)&&list.length())
                 list.append(',');
@@ -3732,22 +3734,18 @@ public:
         afor2.For(width,10,false,true);
         if (afor2.ok) {
             // now rename directory and partmask
-                IPropertyTree &pt = lockProperties(defaultTimeout);
-                root->setProp("@directory",newdir.str());
-                root->setProp("@partmask",newmask.str());
-                partmask.set(newmask.str());
-                directory.set(newdir.str());
-                StringBuffer mask;
-                for (unsigned i=0;i<width;i++) {
-                    mask.appendf("Part[%d]/@name",i+1);
-                    //StringBuffer x;
-                    //pt.getProp(mask.str(),x);
-                    //pt.removeProp(mask.str());
-                    parts.item(i).clearOverrideName();  
-                }
-                savePartsAttr(false);
-                unlockProperties();
-
+            lockProperties(defaultTimeout);
+            root->setProp("@directory",newdir.str());
+            root->setProp("@partmask",newmask.str());
+            partmask.set(newmask.str());
+            directory.set(newdir.str());
+            StringBuffer mask;
+            for (unsigned i=0;i<width;i++) {
+                mask.appendf("Part[%d]/@name",i+1);
+                parts.item(i).clearOverrideName();
+            }
+            savePartsAttr(false);
+            unlockProperties();
         }
         else {
             // attempt recovery
@@ -3953,12 +3951,12 @@ public:
             parent->setFileAccessed(logicalName,dt);
         }
         else {
-            IPropertyTree &prop = lockProperties(defaultTimeout);
+            lockProperties(defaultTimeout);
             if (dt.isNull())
-                prop.removeProp("@accessed");
+                queryProperties().removeProp("@accessed");
             else {
                 StringBuffer str;
-                prop.setProp("@accessed",dt.getString(str).str());
+                queryProperties().setProp("@accessed",dt.getString(str).str());
             }
             unlockProperties();
         }
@@ -5187,9 +5185,7 @@ private:
         else {
             pos = subfiles.ordinality();
             if (pos) {
-                bool reload;
-                lockProperties(reload,defaultTimeout);
-                if (reload)
+                if (lockProperties(defaultTimeout))
                     loadSubFiles(true,transaction,1000*60*10); 
                 pos = subfiles.ordinality();
                 if (pos) {
@@ -5335,11 +5331,9 @@ public:
         Owned<IDistributedFile> sub = transaction ? transaction->lookupFile(subfile) : parent->lookup(subfile,udesc,false,NULL,defaultTimeout);
         if (!sub)
             throw MakeStringException(-1,"addSubFile(3): File %s cannot be found to add",subfile);
-        bool reload;
-        lockProperties(reload,defaultTimeout);
-        // need to reload subfiles if changed
         try {
-            if (reload)
+            // need to reload subfiles if changed
+            if (lockProperties(defaultTimeout))
                 loadSubFiles(true,transaction,1000*60*10);
             doAddSubFile(sub.getClear(),before,other,transaction);
         }
@@ -5780,16 +5774,10 @@ unsigned CDistributedFilePart::getPhysicalCrc()
     throw e;
 }
 
-IPropertyTree &CDistributedFilePart::lockProperties(unsigned timeoutms)
+bool CDistributedFilePart::lockProperties(unsigned timeoutms)
 {
-    parent.lockProperties(timeoutms);
     dirty = true;
-    CriticalBlock block (sect);     // avoid nested blocks
-    if (attr) 
-        return *attr;
-    WARNLOG("CDistributedFilePart::queryProperties missing part attributes");
-    attr.setown(getEmptyAttr());
-    return *attr;
+    return parent.lockProperties(timeoutms);
 }
 
 void CDistributedFilePart::unlockProperties()
@@ -9082,7 +9070,8 @@ bool CDistributedFileDirectory::filePhysicalVerify(const char *lfn,bool includec
                 if (!differs&&!includecrc) {
                     if (nological) {
                         StringBuffer str;
-                        part->lockProperties(defaultTimeout).setProp("@modified",dt2.getString(str).str());
+                        part->lockProperties(defaultTimeout);
+                        part->queryProperties().setProp("@modified",dt2.getString(str).str());
                         part->unlockProperties();
                     }
                     else  {
@@ -9106,7 +9095,8 @@ bool CDistributedFileDirectory::filePhysicalVerify(const char *lfn,bool includec
                     }
                     if (sz1!=sz2) {
                         if (sz1==(offset_t)-1) {
-                            part->lockProperties(defaultTimeout).setPropInt64("@size",sz2);
+                            part->lockProperties(defaultTimeout);
+                            part->queryProperties().setPropInt64("@size",sz2);
                             part->unlockProperties();
                         }
                         else if (sz2!=(offset_t)-1) {
@@ -9127,7 +9117,8 @@ bool CDistributedFileDirectory::filePhysicalVerify(const char *lfn,bool includec
                         crc2 = part->getPhysicalCrc();
                     }
                     if (!part->getCrc(crc1)) {
-                        part->lockProperties(defaultTimeout).setPropInt64("@fileCrc",(unsigned)crc2);
+                        part->lockProperties(defaultTimeout);
+                        part->queryProperties().setPropInt64("@fileCrc",(unsigned)crc2);
                         part->unlockProperties();
                     }
                     else if (crc1!=crc2) {

dali/base/dadfs.hpp

@@ -133,7 +133,7 @@ interface IDistributedFilePart: implements IInterface
 
     virtual IPropertyTree &queryProperties() = 0;                               // part properties
 
-    virtual IPropertyTree &lockProperties(unsigned timeoutms=INFINITE) = 0;                             // must be called before updating
+    virtual bool lockProperties(unsigned timeoutms=INFINITE) = 0;               // must be called before updating
     virtual void unlockProperties() = 0;                                        // must be called after updating
 
     virtual bool isHost(unsigned copy=0) = 0;                                   // file is located on this machine
@@ -219,7 +219,7 @@ interface IDistributedFile: extends IInterface
 
     virtual IPropertyTree &queryProperties() = 0;                               // DFile attributes (TODO: rename to getFileAttr)
 
-    virtual IPropertyTree &lockProperties(unsigned timeoutms=INFINITE) = 0;     // must be called before updating properties (will discard uncommitted changes)
+    virtual bool lockProperties(unsigned timeoutms=INFINITE) = 0;               // must be called before updating properties (will discard uncommitted changes)
     virtual void unlockProperties() = 0;                                        // must be called after updating properties
 
     virtual bool getModificationTime(CDateTime &dt) = 0;                        // get date and time last modified (returns false if not set)

dali/datest/datest.cpp

@@ -76,7 +76,8 @@ static void addTestFile(const char *name,unsigned n)
         fileDesc->setPart(m,&group->queryNode(m), path.str(), pp);
     }
     Owned<IDistributedFile> dfile =  queryDistributedFileDirectory().createNew(fileDesc);
-    IPropertyTree &t = dfile->lockProperties();
+    dfile->lockProperties();
+    IPropertyTree &t = dfile->queryProperties();
     t.setProp("@owned","nigel");
     t.setPropInt("@recordSize",1);
     t.setProp("ECL","TESTECL();");

dali/dfu/dfurun.cpp

@@ -903,7 +903,8 @@ public:
                 sfile->addSubFile(subfiles.item(i));
             }
             if (newroxieprefix.length()) {
-                sfile->lockProperties().setProp("@roxiePrefix",newroxieprefix.str());
+                sfile->lockProperties();
+                sfile->queryProperties().setProp("@roxiePrefix",newroxieprefix.str());
                 sfile->unlockProperties();
             }
 

dali/dfu/dfuutil.cpp

@@ -636,7 +636,8 @@ public:
                 sfile->addSubFile(subfiles.item(i));
             }
             if (!nameprefix.isEmpty()) {
-                sfile->lockProperties().setProp("@roxiePrefix",nameprefix.get());
+                sfile->lockProperties();
+                sfile->queryProperties().setProp("@roxiePrefix",nameprefix.get());
                 sfile->unlockProperties();
             }
         }

dali/ft/daft.cpp

@@ -205,8 +205,8 @@ offset_t CDistributedFileSystem::getSize(IDistributedFile * file, bool forceget,
         }
         if (((totalSize != -1)||forceget) && !dontsetattr) // note forceget && !dontsetattr will reset attr if can't work out size
         {
-            IPropertyTree &tree = file->lockProperties();
-            tree.setPropInt64("@size", totalSize);
+            file->lockProperties();
+            file->queryProperties().setPropInt64("@size", totalSize);
             file->unlockProperties();
         }
     }
@@ -271,8 +271,8 @@ offset_t CDistributedFileSystem::getSize(IDistributedFilePart * part, bool force
         size = part->getFileSize(true,forceget);
         if (((size != (offset_t)-1)||forceget) && !dontsetattr) // note forceget && !dontsetattr will reset attr if can't work out size
         {
-            IPropertyTree &tree = part->lockProperties();
-            tree.setPropInt64("@size", size);
+            part->lockProperties();
+            part->queryProperties().setPropInt64("@size", size);
             part->unlockProperties();
         }
     }

dali/ft/filecopy.cpp

@@ -1481,7 +1481,8 @@ void FileSprayer::analyseFileHeaders(bool setcurheadersize)
         tgtFormat.set(srcFormat);
         if (distributedTarget)
         {
-            IPropertyTree &curProps = distributedTarget->lockProperties();
+            distributedTarget->lockProperties();
+            IPropertyTree &curProps = distributedTarget->queryProperties();
             tgtFormat.save(&curProps);
             distributedTarget->unlockProperties();
         }
@@ -2427,7 +2428,8 @@ void FileSprayer::setTarget(IDistributedFile * target)
         tgtFormat.set(srcFormat);
         if (!unknownSourceFormat)
         {
-            IPropertyTree &curProps = target->lockProperties();
+            target->lockProperties();
+            IPropertyTree &curProps = target->queryProperties();
             tgtFormat.save(&curProps);
             target->unlockProperties();
         }
@@ -2758,7 +2760,8 @@ void FileSprayer::updateTargetProperties()
             if (idx+1 == partition.ordinality() || partition.item(idx+1).whichOutput != cur.whichOutput)
             {
                 Owned<IDistributedFilePart> curPart = distributedTarget->getPart(cur.whichOutput);
-                IPropertyTree& curProps = curPart->lockProperties();
+                curPart->lockProperties();
+                IPropertyTree& curProps = curPart->queryProperties();
                 if (calcCRC())
                 {
                     curProps.setPropInt(FAcrc, partCRC.get());
@@ -2827,7 +2830,8 @@ void FileSprayer::updateTargetProperties()
         if (failedParts.length())
             error.setown(MakeStringException(DFTERR_InputOutputCrcMismatch, "%s", failedParts.str()));
 
-        IPropertyTree &curProps = distributedTarget->lockProperties();
+        distributedTarget->lockProperties();
+        IPropertyTree &curProps = distributedTarget->queryProperties();
         if (calcCRC())
             curProps.setPropInt(FAcrc, totalCRC.get());
         curProps.setPropInt64(FAsize, totalLength);

dali/regress/daregdfs.cpp

@@ -586,7 +586,8 @@ void testDF1()
     fdesc->setPart(2,rfn,pt);
     dispFDesc(fdesc);
     Owned<IDistributedFile> file = queryDistributedFileDirectory().createNew(fdesc);
-    file->lockProperties().setProp("@testing","1");
+    file->lockProperties();
+    file->queryProperties().setProp("@testing","1");
     file->unlockProperties();
     file->attach("testing::propfile2");
 }

dali/sasha/saverify.cpp

@@ -349,9 +349,9 @@ public:
             if (afor.ok) {
                 CDateTime dt;
                 dt.setNow();
-                IPropertyTree &pt = file->lockProperties();
+                file->lockProperties();
                 StringBuffer str;
-                pt.setProp("@verified",dt.getString(str).str());
+                file->queryProperties().setProp("@verified",dt.getString(str).str());
                 file->unlockProperties();
             }
             PROGLOG("VERIFY: file %s %s",name,afor.ok?"OK":"FAILED");

ecl/hthor/hthorkey.cpp

@@ -1928,8 +1928,8 @@ protected:
                     partsize = ifile->size();
                     if (partsize != -1)
                     {
-                        IPropertyTree &tree = part->lockProperties();
-                        tree.setPropInt64("@size", partsize);
+                        part->lockProperties();
+                        part->queryProperties().setPropInt64("@size", partsize);
                         part->unlockProperties();
                         break;
                     }

esp/services/ws_dfu/ws_dfuService.cpp

@@ -1723,7 +1723,8 @@ void CWsDfuEx::doGetFileDetails(IEspContext &context, IUserDescriptor* udesc, co
     StringBuffer strDesc = df->queryProperties().queryProp("@description");
     if (description)
     {
-        df->lockProperties().setProp("@description",description); 
+        df->lockProperties();
+        df->queryProperties().setProp("@description",description);
         df->unlockProperties();
         strDesc = description;
     }

plugins/fileservices/fileservices.cpp

@@ -1439,7 +1439,8 @@ FILESERVICES_API void FILESERVICES_CALL fsSetFileDescription(ICodeContext *ctx,
     Linked<IUserDescriptor> udesc = ctx->queryUserDescriptor();
     Owned<IDistributedFile> df = queryDistributedFileDirectory().lookup(lfn.str(),udesc);
     if (df) {
-        df->lockProperties().setProp("@description",value);
+        df->lockProperties();
+        df->queryProperties().setProp("@description",value);
         df->unlockProperties();
     }
     else

thorlcr/activities/keydiff/thkeydiff.cpp

@@ -181,7 +181,8 @@ public:
             e->Release();
         }
         // Add a new 'Patch' description to the secondary key.
-        IPropertyTree &fileProps = newIndexFile->lockProperties();
+        newIndexFile->lockProperties();
+        IPropertyTree &fileProps = newIndexFile->queryProperties();
         StringBuffer path("Patch[@name=\"");
         path.append(scopedName.str()).append("\"]");
         IPropertyTree *patch = fileProps.queryPropTree(path.str());