ENH: more consistent single-ownership when swapping fileHandlers

- make fileHandler deletion mechanism more
  transparent by providing a nullptr signature. A nullptr parameter
  is already being used in the argList destructor for shutdown, but that
  relied on an implicit conversion to autoPtr to trigger things.

- improved handling of file handler replacement.

  Previously had a very basic check on old vs new handlers using their
  type() values (string comparison!!), which would unfortunately
  prevent proper swapping of the contents.
  Check the actual pointers instead.

  As part of the change, treat any empty autoPtr as no-op instead of as
  deletion (which is handled explicitly as nullptr instead).

  In addition to making the internal logic simpler, it means that the
  current file handler always changes to a valid state without
  inadvertently removing everything and falling back to creating a new
  default handler (again).

  This handling of no-ops also simplifies call code. For example,

  <code>
      autoPtr<fileHandler> oldHandler;
      autoPtr<fileHandler> writeHandler;
      word handlerName;

      if (arg.readIfPresent("writeHandler", handlerName))
      {
          writeHandler = fileOperation::New(handlerName);
      }

      oldHandler = fileHandler(std::move(writeHandler));

      ... do something

      writeHandler = fileHandler(std::move(oldHandler));
  </code>

  If the "writeHandler" is not specified, each call is a no-op.
  If it is specified, the handlers are swapped out each time.

- the management of the fileHandler communicators is now encapsulated
  privately (managedComm_) with the final layer being responsible for
  cleaning up after itself. This makes delegation/inheritance clearer
  and avoids the risk of freeing an MPI communicator twice.

STYLE: uniformFile static check relocated to fileOperation layer

src/OpenFOAM/db/IOstreams/Fstreams/masterOFstream.C

@@ -96,11 +96,7 @@ void Foam::masterOFstream::commit()
         filePaths[Pstream::myProcNo()] = pathName_;
         Pstream::gatherList(filePaths);
 
-        bool uniform =
+        bool uniform = fileOperation::uniformFile(filePaths);
-            fileOperations::masterUncollatedFileOperation::uniformFile
-            (
-                filePaths
-            );
 
         Pstream::broadcast(uniform);
 

src/OpenFOAM/global/argList/argList.C

@@ -1815,7 +1815,7 @@ Foam::argList::~argList()
     jobInfo.stop();     // Normal job termination
 
     // Delete file handler to flush any remaining IO
-    Foam::fileHandler(nullptr);
+    (void)Foam::fileHandler(nullptr);
 }
 
 

src/OpenFOAM/global/fileOperations/collatedFileOperation/collatedFileOperation.C

@@ -287,7 +287,7 @@ Foam::fileOperations::collatedFileOperation::collatedFileOperation
         ),
         false
     ),
-    myComm_(comm_),
+    managedComm_(comm_),
     writer_(mag(maxThreadFileBufferSize), comm_),
     nProcs_(Pstream::nProcs()),
     ioRanks_(ioRanks())
@@ -305,7 +305,7 @@ Foam::fileOperations::collatedFileOperation::collatedFileOperation
 )
 :
     masterUncollatedFileOperation(comm, false),
-    myComm_(-1),
+    managedComm_(-1),  // Externally managed
     writer_(mag(maxThreadFileBufferSize), comm),
     nProcs_(Pstream::nProcs()),
     ioRanks_(ioRanks)
@@ -321,9 +321,9 @@ Foam::fileOperations::collatedFileOperation::~collatedFileOperation()
     // Wait for any outstanding file operations
     flush();
 
-    if (myComm_ != -1 && myComm_ != UPstream::worldComm)
+    if (UPstream::isUserComm(managedComm_))
     {
-        UPstream::freeCommunicator(myComm_);
+        UPstream::freeCommunicator(managedComm_);
     }
 }
 

src/OpenFOAM/global/fileOperations/collatedFileOperation/collatedFileOperation.H

@@ -67,6 +67,12 @@ class collatedFileOperation
 :
     public masterUncollatedFileOperation
 {
+    // Private Data
+
+        //- Communicator allocated/managed by us
+        label managedComm_;
+
+
     // Private Member Functions
 
         //- Any initialisation steps after constructing
@@ -76,9 +82,6 @@ protected:
 
     // Protected Data
 
-        //- Any communicator allocated by me
-        const label myComm_;
-
         //- Threaded writer
         mutable OFstreamCollator writer_;
 

src/OpenFOAM/global/fileOperations/collatedFileOperation/hostCollatedFileOperation.C

@@ -149,7 +149,8 @@ Foam::fileOperations::hostCollatedFileOperation::hostCollatedFileOperation
         (Pstream::parRun() ? labelList() : ioRanks()), // processor dirs
         typeName,
         false // verbose
-    )
+    ),
+    managedComm_(comm_)
 {
     init(verbose);
 }
@@ -159,9 +160,9 @@ Foam::fileOperations::hostCollatedFileOperation::hostCollatedFileOperation
 
 Foam::fileOperations::hostCollatedFileOperation::~hostCollatedFileOperation()
 {
-    if (comm_ != -1 && comm_ != UPstream::worldComm)
+    if (UPstream::isUserComm(managedComm_))
     {
-        UPstream::freeCommunicator(comm_);
+        UPstream::freeCommunicator(managedComm_);
     }
 }
 

src/OpenFOAM/global/fileOperations/collatedFileOperation/hostCollatedFileOperation.H

@@ -6,7 +6,7 @@
      \\/     M anipulation  |
 -------------------------------------------------------------------------------
     Copyright (C) 2017-2018 OpenFOAM Foundation
-    Copyright (C) 2021 OpenCFD Ltd.
+    Copyright (C) 2021-2022 OpenCFD Ltd.
 -------------------------------------------------------------------------------
 License
     This file is part of OpenFOAM.
@@ -77,6 +77,12 @@ class hostCollatedFileOperation
 :
     public collatedFileOperation
 {
+    // Private Data
+
+        //- Communicator allocated/managed by us
+        label managedComm_;
+
+
     // Private Member Functions
 
         //- Any initialisation steps after constructing

src/OpenFOAM/global/fileOperations/fileOperation/fileOperation.C

@@ -295,6 +295,42 @@ Foam::fileOperation::sortTimes
 }
 
 
+bool Foam::fileOperation::uniformFile(const fileNameList& names)
+{
+    if (names.empty())
+    {
+        return false;
+    }
+
+    const auto& object0 = names[0];
+
+    for (label i = 1; i < names.size(); ++i)
+    {
+        if (object0 != names[i])
+        {
+            return false;
+        }
+    }
+
+    return true;
+}
+
+
+bool Foam::fileOperation::uniformFile(const label comm, const fileName& name)
+{
+    if (!Pstream::parRun())
+    {
+        return true;
+    }
+
+    fileName masterName(name);
+
+    Pstream::broadcast(masterName, comm);
+
+    return returnReduceAnd((masterName == name), comm);
+}
+
+
 // * * * * * * * * * * * * Protected Member Functions  * * * * * * * * * * * //
 
 Foam::fileMonitor& Foam::fileOperation::monitor() const
@@ -710,6 +746,18 @@ Foam::fileOperation::New
     bool verbose
 )
 {
+    if (handlerType.empty())
+    {
+        if (fileOperation::defaultFileHandler.empty())
+        {
+            FatalErrorInFunction
+                << "defaultFileHandler name is undefined" << nl
+                << abort(FatalError);
+        }
+
+        return fileOperation::New(fileOperation::defaultFileHandler, verbose);
+    }
+
     DebugInFunction
         << "Constructing fileHandler" << endl;
 
@@ -1511,37 +1559,56 @@ const Foam::fileOperation& Foam::fileHandler()
 {
     if (!fileOperation::fileHandlerPtr_)
     {
-        word handler(Foam::getEnv("FOAM_FILEHANDLER"));
+        word handlerType(Foam::getEnv("FOAM_FILEHANDLER"));
 
-        if (handler.empty())
+        if (handlerType.empty())
         {
-            handler = fileOperation::defaultFileHandler;
+            handlerType = fileOperation::defaultFileHandler;
         }
 
-        fileOperation::fileHandlerPtr_ = fileOperation::New(handler, true);
+        fileOperation::fileHandlerPtr_ = fileOperation::New(handlerType, true);
     }
 
     return *fileOperation::fileHandlerPtr_;
 }
 
 
+Foam::autoPtr<Foam::fileOperation>
+Foam::fileHandler(std::nullptr_t)
+{
+    return autoPtr<fileOperation>(fileOperation::fileHandlerPtr_.release());
+}
+
+
 Foam::autoPtr<Foam::fileOperation>
 Foam::fileHandler(autoPtr<fileOperation>&& newHandler)
 {
+    // - do nothing if newHandler is empty. Does not delete current
+    // - do nothing if newHandler is identical to current handler
+
+    // Change ownership as atomic operations
+
+    // If newHandler and current handler are actually identical, we
+    // have a bit problem somewhere else since this means that the pointer
+    // is managed is done in two places!
+    // Should flag as a FatalError (in the future), but there may still be
+    // some place where we would like to fake shared pointers?
+
+    // TBD: add a flush() operation on the old handler first,
+    // instead of waiting for it to be run on destruction?
+
+    autoPtr<fileOperation> old;
+
     if
     (
-        newHandler
+        newHandler.get() != nullptr
-     && fileOperation::fileHandlerPtr_
+     && newHandler.get() != fileOperation::fileHandlerPtr_.get()
-     && newHandler->type() == fileOperation::fileHandlerPtr_->type()
     )
     {
-        return nullptr;  // No change
+        old.reset(newHandler.release());
+        old.swap(fileOperation::fileHandlerPtr_);
     }
 
-    autoPtr<fileOperation> old(std::move(fileOperation::fileHandlerPtr_));
-
-    fileOperation::fileHandlerPtr_ = std::move(newHandler);
-
     return old;
 }
 

src/OpenFOAM/global/fileOperations/fileOperation/fileOperation.H

@@ -196,6 +196,9 @@ public:
             const bool distributedRoots = false
         );
 
+        //- Clone fileHandler
+        /// virtual autoPtr<fileOperation> clone() const = 0;
+
 
     // Declare run-time constructor selection table
 
@@ -213,7 +216,8 @@ public:
 
     // Selectors
 
-        //- Select fileHandler-type
+        //- Select fileHandler-type.
+        //- Uses defaultFileHandler if the handlerType is empty.
         static autoPtr<fileOperation> New
         (
             const word& handlerType,
@@ -235,6 +239,12 @@ public:
             const word& constantName = "constant"
         );
 
+        //- True if the file names are identical. False on an empty list
+        static bool uniformFile(const fileNameList& names);
+
+        //- True if the file name is identical on all ranks
+        static bool uniformFile(const label comm, const fileName& name);
+
 
     // Member Functions
 
@@ -628,11 +638,21 @@ inline Ostream& operator<<(Ostream& os, const fileOperation::pathType b)
 
 // Note: defined in fileOperation.C
 
-//- Get current file handler
+//- Return the current file handler.
+//- Will create the default file handler if necessary.
 const fileOperation& fileHandler();
 
-//- Replace, reset file handler.
+//- Delete current file handler.
-//  \return old handler on change, null otherwise
+//  \returns the old handler
+autoPtr<fileOperation> fileHandler(std::nullptr_t);
+
+//- Replace the current file handler.
+//  The following are considered no-ops:
+//  - an empty/invalid newHandler does \b not delete,
+//    use fileHandler(std::nullptr_t) - ie, a literal \c nullptr for that
+//  - if new handler and current handler are identical (same pointer).
+//  .
+//  \returns the old handler (on change), nullptr otherwise
 autoPtr<fileOperation> fileHandler(autoPtr<fileOperation>&& newHandler);
 
 

src/OpenFOAM/global/fileOperations/masterUncollatedFileOperation/masterUncollatedFileOperation.C

@@ -474,24 +474,6 @@ Foam::fileOperations::masterUncollatedFileOperation::localObjectPath
 }
 
 
-bool Foam::fileOperations::masterUncollatedFileOperation::uniformFile
-(
-    const fileNameList& filePaths
-)
-{
-    const fileName& object0 = filePaths[0];
-
-    for (label i = 1; i < filePaths.size(); i++)
-    {
-        if (filePaths[i] != object0)
-        {
-            return false;
-        }
-    }
-    return true;
-}
-
-
 void Foam::fileOperations::masterUncollatedFileOperation::readAndSend
 (
     const fileName& filePath,
@@ -763,7 +745,7 @@ masterUncollatedFileOperation
             subRanks(Pstream::nProcs())
         )
     ),
-    myComm_(comm_)
+    managedComm_(comm_)
 {
     init(verbose);
 }
@@ -777,7 +759,7 @@ masterUncollatedFileOperation
 )
 :
     fileOperation(comm),
-    myComm_(-1)
+    managedComm_(-1)  // Externally managed
 {
     init(verbose);
 }
@@ -788,9 +770,9 @@ masterUncollatedFileOperation
 Foam::fileOperations::masterUncollatedFileOperation::
 ~masterUncollatedFileOperation()
 {
-    if (myComm_ != -1 && myComm_ != UPstream::worldComm)
+    if (UPstream::isUserComm(managedComm_))
     {
-        UPstream::freeCommunicator(myComm_);
+        UPstream::freeCommunicator(managedComm_);
     }
 }
 

src/OpenFOAM/global/fileOperations/masterUncollatedFileOperation/masterUncollatedFileOperation.H

@@ -88,6 +88,12 @@ class masterUncollatedFileOperation
 :
     public fileOperation
 {
+    // Private Data
+
+        //- Communicator allocated/managed by us
+        label managedComm_;
+
+
     // Private Member Functions
 
         //- Any initialisation steps after constructing
@@ -97,9 +103,6 @@ protected:
 
     // Protected Data
 
-        //- Any communicator allocated by me
-        const label myComm_;
-
         //- Cached times for a given directory
         mutable HashPtrTable<DynamicList<instant>> times_;
 
@@ -744,9 +747,6 @@ public:
 
         // Other
 
-            //- Same file?
-            static bool uniformFile(const fileNameList&);
-
             //- Get sorted list of times
             virtual instantList findTimes(const fileName&, const word&) const;
 

src/OpenFOAM/global/fileOperations/uncollatedFileOperation/uncollatedFileOperation.C

@@ -193,12 +193,24 @@ Foam::fileOperations::uncollatedFileOperation::uncollatedFileOperation
     bool verbose
 )
 :
-    fileOperation(Pstream::worldComm)
+    fileOperation(UPstream::worldComm),
+    managedComm_(-1)  // worldComm is externally managed
 {
     init(verbose);
 }
 
 
+// * * * * * * * * * * * * * * * * Destructor  * * * * * * * * * * * * * * * //
+
+Foam::fileOperations::uncollatedFileOperation::~uncollatedFileOperation()
+{
+    if (UPstream::isUserComm(managedComm_))
+    {
+        UPstream::freeCommunicator(managedComm_);
+    }
+}
+
+
 // * * * * * * * * * * * * * Filesystem Operations * * * * * * * * * * * * * //
 
 bool Foam::fileOperations::uncollatedFileOperation::mkDir

src/OpenFOAM/global/fileOperations/uncollatedFileOperation/uncollatedFileOperation.H

@@ -53,6 +53,12 @@ class uncollatedFileOperation
 :
     public fileOperation
 {
+    // Private Data
+
+        //- Communicator allocated/managed by us
+        label managedComm_;
+
+
     // Private Member Functions
 
         //- Any initialisation steps after constructing
@@ -95,7 +101,7 @@ public:
 
 
     //- Destructor
-    virtual ~uncollatedFileOperation() = default;
+    virtual ~uncollatedFileOperation();
 
 
     // Member Functions