Improve const correctness of Principal internals

Reduced the number of mutable data members used.
Switched to using propagate_const<> to make pointer like data members have deep const behavior.
Changes the API of a number of internal classes to be properly const or non-const.
These changes are meant to make it easier to reason about the thread-safety of the classes.

DataFormats/Common/interface/BasicHandle.h

@@ -53,8 +53,8 @@ namespace edm {
       whyFailedFactory_(h.whyFailedFactory_){}
 
     explicit BasicHandle(ProductData const& productData) :
-      product_(productData.wrapper_.get()),
-      prov_(&productData.prov_) {
+      product_(productData.wrapper()),
+      prov_(&productData.provenance()) {
     }
 
 #if defined( __GXX_EXPERIMENTAL_CXX0X__)

DataFormats/Common/interface/OutputHandle.h

@@ -49,7 +49,7 @@ namespace edm {
       productProvenance_(h.productProvenance_),
       whyFailed_(h.whyFailed_){}
 
-    OutputHandle(WrapperBase const* product, BranchDescription const* desc, ProductProvenance* productProvenance) :
+    OutputHandle(WrapperBase const* product, BranchDescription const* desc, ProductProvenance const* productProvenance) :
       product_(product),
       desc_(desc),
       productProvenance_(productProvenance) {}
@@ -105,7 +105,7 @@ namespace edm {
   private:
     WrapperBase const* product_;
     BranchDescription const* desc_;
-    ProductProvenance* productProvenance_;
+    ProductProvenance const* productProvenance_;
     std::shared_ptr<cms::Exception> whyFailed_;
   };
 

DataFormats/Common/interface/ProductData.h

@@ -9,12 +9,14 @@ is the storage unit of such information.
 ----------------------------------------------------------------------*/
 
 #include "DataFormats/Provenance/interface/Provenance.h"
+#include "FWCore/Utilities/interface/propagate_const.h"
 #include <memory>
 
 namespace edm {
   class BranchDescription;
   class WrapperBase;
-  struct ProductData {
+  class ProductData {
+  public:
     ProductData();
 
     explicit ProductData(std::shared_ptr<BranchDescription const> bd);
@@ -25,26 +27,69 @@ namespace edm {
     std::shared_ptr<BranchDescription const> const& branchDescription() const {
       return prov_.constBranchDescriptionPtr();
     }
-
+
+    Provenance const& provenance() const { return prov_;}
+
+    WrapperBase const* wrapper() const { return wrapper_.get();}
+    WrapperBase* wrapper() { return wrapper_.get(); }
+    std::shared_ptr<WrapperBase const> sharedConstWrapper() const {
+      return wrapper_;
+    }
+
     void swap(ProductData& other) {
        std::swap(wrapper_, other.wrapper_);
        prov_.swap(other.prov_);
     }
 
+    void setWrapper(std::unique_ptr<WrapperBase> iValue);
+
+    //Not const thread-safe update
+    void unsafe_setWrapper(std::unique_ptr<WrapperBase> iValue) const;
+
     void resetBranchDescription(std::shared_ptr<BranchDescription const> bd);
 
     void resetProductData() {
       wrapper_.reset();
       prov_.resetProductProvenance();
     }
+
+    void setProcessHistory(ProcessHistory const& ph) {
+      prov_.setProcessHistory(ph);
+    }
+
+    void setProvenance(ProductProvenanceRetriever const* provRetriever, ProcessHistory const& ph, ProductID const& pid) {
+      prov_.setProductID(pid);
+      prov_.setStore(provRetriever);
+      prov_.setProcessHistory(ph);
+    }
+
+    void setProductProvenance(ProductProvenance const& prov ) {
+      prov_.setProductProvenance(prov);
+    }
+
+    void connectTo( ProductData const& iOther) {
+      wrapper_ = iOther.wrapper_;
+      // Then the product ID and the ProcessHistory
+      prov_.setProductID(iOther.prov_.productID());
+      prov_.setProcessHistory(iOther.prov_.processHistory());
+      // Then the store, in case the product needs reading in a subprocess.
+      prov_.setStore(iOther.prov_.store());
+      // And last, the other per event provenance.
+      if(iOther.prov_.productProvenanceValid()) {
+        prov_.setProductProvenance(*iOther.prov_.productProvenance());
+      } else {
+        prov_.resetProductProvenance();
+      }
 
+    }
     // NOTE: We should probably think hard about whether these
     // variables should be declared "mutable" as part of
     // the effort to make the Framework multithread capable ...
 
+  private:
     // "non-const data" (updated every event)
     mutable std::shared_ptr<WrapperBase> wrapper_;
-    mutable Provenance prov_;
+    Provenance prov_;
   };
 
   // Free swap function

DataFormats/Common/src/ProductData.cc

@@ -3,6 +3,7 @@
 #include "DataFormats/Common/interface/ProductData.h"
 
 #include "DataFormats/Provenance/interface/ProductID.h"
+#include "DataFormats/Common/interface/WrapperBase.h"
 #include "FWCore/Utilities/interface/do_nothing_deleter.h"
 
 namespace edm {
@@ -26,4 +27,13 @@ namespace edm {
   ProductData::resetBranchDescription(std::shared_ptr<BranchDescription const> bd) {
     prov_.setBranchDescription(bd);
   }
+
+  void ProductData::setWrapper(std::unique_ptr<WrapperBase> iValue) {
+    wrapper_ = std::move(iValue);
+  }
+
+  //Not const thread-safe update
+  void ProductData::unsafe_setWrapper(std::unique_ptr<WrapperBase> iValue) const {
+    wrapper_ = std::move(iValue);
+  }
 }

DataFormats/Provenance/interface/ProductProvenanceRetriever.h

@@ -9,11 +9,8 @@ ProductProvenanceRetriever: Manages the per event/lumi/run per product provenanc
 #include "DataFormats/Provenance/interface/BranchID.h"
 #include "DataFormats/Provenance/interface/ProductProvenance.h"
 #include "DataFormats/Provenance/interface/ProcessHistoryID.h"
+#include "FWCore/Utilities/interface/propagate_const.h"
 
-#include "boost/scoped_ptr.hpp"
-#include "boost/utility.hpp"
-
-#include <iosfwd>
 #include <memory>
 #include <set>
 
@@ -24,10 +21,12 @@ ProductProvenanceRetriever: Manages the per event/lumi/run per product provenanc
 namespace edm {
   class ProvenanceReaderBase;
 
-  class ProductProvenanceRetriever : private boost::noncopyable {
+  class ProductProvenanceRetriever {
   public:
     explicit ProductProvenanceRetriever(unsigned int iTransitionIndex);
     explicit ProductProvenanceRetriever(std::unique_ptr<ProvenanceReaderBase> reader);
+
+    ProductProvenanceRetriever& operator=(ProductProvenanceRetriever const&) = delete;
 
     ~ProductProvenanceRetriever();
 
@@ -37,7 +36,7 @@ namespace edm {
 
     void mergeProvenanceRetrievers(std::shared_ptr<ProductProvenanceRetriever> other);
 
-    void deepSwap(ProductProvenanceRetriever&);
+    void deepCopy(ProductProvenanceRetriever const&);
 
     void reset();
   private:
@@ -49,8 +48,8 @@ namespace edm {
     typedef std::set<ProductProvenance> eiSet;
 
     mutable eiSet entryInfoSet_;
-    std::shared_ptr<ProductProvenanceRetriever> nextRetriever_;
-    mutable std::shared_ptr<ProvenanceReaderBase> provenanceReader_;
+    edm::propagate_const<std::shared_ptr<ProductProvenanceRetriever>> nextRetriever_;
+    std::shared_ptr<const ProvenanceReaderBase> provenanceReader_;
     unsigned int transitionIndex_;
     mutable bool delayedRead_;
   };

DataFormats/Provenance/src/ProductProvenanceRetriever.cc

@@ -38,18 +38,18 @@ namespace edm {
     }
   }
 
-  void ProductProvenanceRetriever::deepSwap(ProductProvenanceRetriever& iFrom)
+  void ProductProvenanceRetriever::deepCopy(ProductProvenanceRetriever const& iFrom)
   {
-    entryInfoSet_.swap(iFrom.entryInfoSet_);
+    entryInfoSet_ = iFrom.entryInfoSet_;
     provenanceReader_ = iFrom.provenanceReader_;
     if(provenanceReader_) {
       delayedRead_=true;
     }
     if(iFrom.nextRetriever_) {
       if(not nextRetriever_) {
-        nextRetriever_.reset(new ProductProvenanceRetriever(transitionIndex_));
+        get_underlying(nextRetriever_) = std::make_shared<ProductProvenanceRetriever>(transitionIndex_);
       }
-      nextRetriever_->deepSwap(*(iFrom.nextRetriever_));
+      nextRetriever_->deepCopy(*(iFrom.nextRetriever_));
     }
   }
 

FWCore/Framework/interface/EventPrincipal.h

@@ -72,7 +72,7 @@ namespace edm {
                             ProcessHistoryRegistry const& processHistoryRegistry,
                             EventSelectionIDVector&& eventSelectionIDs,
                             BranchListIndexes&& branchListIndexes,
-                            ProductProvenanceRetriever& provRetriever,
+                            ProductProvenanceRetriever const& provRetriever,
                             DelayedReader* reader = nullptr);
 
 
@@ -134,10 +134,10 @@ namespace edm {
 
     RunPrincipal const& runPrincipal() const;
 
-    std::shared_ptr<ProductProvenanceRetriever> productProvenanceRetrieverPtr() const {return provRetrieverPtr_;}
+    ProductProvenanceRetriever const* productProvenanceRetrieverPtr() const {return provRetrieverPtr_.get();}
 
     void setUnscheduledHandler(std::shared_ptr<UnscheduledHandler> iHandler);
-    std::shared_ptr<UnscheduledHandler> unscheduledHandler() const;
+    std::shared_ptr<const UnscheduledHandler> unscheduledHandler() const;
 
     EventSelectionIDVector const& eventSelectionIDs() const;
 
@@ -168,7 +168,7 @@ namespace edm {
     ProductID branchIDToProductID(BranchID const& bid) const;
 
     void mergeProvenanceRetrievers(EventPrincipal const& other) {
-      provRetrieverPtr_->mergeProvenanceRetrievers(other.productProvenanceRetrieverPtr());
+      provRetrieverPtr_->mergeProvenanceRetrievers(get_underlying(other.provRetrieverPtr_));
     }
 
     using Base::getProvenance;
@@ -195,13 +195,13 @@ namespace edm {
 
     EventAuxiliary aux_;
 
-    std::shared_ptr<LuminosityBlockPrincipal> luminosityBlockPrincipal_;
+    edm::propagate_const<std::shared_ptr<LuminosityBlockPrincipal>> luminosityBlockPrincipal_;
 
     // Pointer to the 'retriever' that will get provenance information from the persistent store.
-    std::shared_ptr<ProductProvenanceRetriever> provRetrieverPtr_;
+    edm::propagate_const<std::shared_ptr<ProductProvenanceRetriever>> provRetrieverPtr_;
 
     // Handler for unscheduled modules
-    std::shared_ptr<UnscheduledHandler> unscheduledHandler_;
+    std::shared_ptr<UnscheduledHandler const> unscheduledHandler_;
 
     EventSelectionIDVector eventSelectionIDs_;
 

FWCore/Framework/interface/LuminosityBlockPrincipal.h

@@ -17,6 +17,7 @@ is the DataBlock.
 #include "DataFormats/Provenance/interface/RunID.h"
 #include "FWCore/Utilities/interface/LuminosityBlockIndex.h"
 #include "FWCore/Framework/interface/Principal.h"
+#include "FWCore/Utilities/interface/propagate_const.h"
 
 #include <memory>
 
@@ -100,7 +101,7 @@ namespace edm {
         std::unique_ptr<WrapperBase> edp);
 
 
-    void readImmediate() const;
+    void readImmediate();
 
     void setComplete() {
       complete_ = true;
@@ -116,11 +117,11 @@ namespace edm {
 
     virtual unsigned int transitionIndex_() const override;
 
-    void resolveProductImmediate(ProductHolderBase const& phb) const;
+    void resolveProductImmediate(ProductHolderBase& phb);
 
-    std::shared_ptr<RunPrincipal> runPrincipal_;
+    edm::propagate_const<std::shared_ptr<RunPrincipal>> runPrincipal_;
 
-    std::shared_ptr<LuminosityBlockAuxiliary> aux_;
+    edm::propagate_const<std::shared_ptr<LuminosityBlockAuxiliary>> aux_;
 
     LuminosityBlockIndex index_;
 

FWCore/Framework/interface/Principal.h

@@ -29,6 +29,7 @@ pointer to a ProductHolder, when queried.
 #include "FWCore/Framework/interface/ProductHolder.h"
 #include "FWCore/Utilities/interface/InputTag.h"
 #include "FWCore/Utilities/interface/ProductKindOfType.h"
+#include "FWCore/Utilities/interface/propagate_const.h"
 
 #include "boost/iterator/filter_iterator.hpp"
 
@@ -156,6 +157,10 @@ namespace edm {
     // merge Principals containing different products.
     void recombine(Principal& other, std::vector<BranchID> const& bids);
 
+    ProductHolderBase* getModifiableProductHolder(BranchID const& oid) {
+      return const_cast<ProductHolderBase*>( const_cast<const Principal*>(this)->getProductHolder(oid));
+    }
+
     size_t size() const;
 
     // These iterators skip over any null shared pointers
@@ -211,9 +216,9 @@ namespace edm {
     // throws if the pointed to product is already in the Principal.
     void checkUniquenessAndType(WrapperBase const* prod, ProductHolderBase const* productHolder) const;
 
-    void putOrMerge(std::unique_ptr<WrapperBase> prod, ProductHolderBase const* productHolder) const;
+    void putOrMerge(std::unique_ptr<WrapperBase> prod, ProductHolderBase* productHolder);
 
-    void putOrMerge(std::unique_ptr<WrapperBase> prod, ProductProvenance& prov, ProductHolderBase* productHolder);
+    void putOrMerge(std::unique_ptr<WrapperBase> prod, ProductProvenance&& prov, ProductHolderBase* productHolder);
 
   private:
 
@@ -278,7 +283,7 @@ namespace edm {
     // In use cases where the new process should not be appended to
     // input ProcessHistory, the following pointer should be null.
     // The Principal does not own this object.
-    HistoryAppender* historyAppender_;
+    edm::propagate_const<HistoryAppender*> historyAppender_;
 
     CacheIdentifier_t cacheIdentifier_;
 
@@ -294,10 +299,10 @@ namespace edm {
       return std::shared_ptr<Wrapper<PROD> const>(); 
     }
 
-    if(!(result->wrapper_->dynamicTypeInfo() == typeid(PROD))) {
-      handleimpl::throwConvertTypeError(typeid(PROD), result->wrapper_->dynamicTypeInfo());
+    if(!(result->wrapper()->dynamicTypeInfo() == typeid(PROD))) {
+      handleimpl::throwConvertTypeError(typeid(PROD), result->wrapper()->dynamicTypeInfo());
     }
-    return std::static_pointer_cast<Wrapper<PROD> const>(result->wrapper_);
+    return std::static_pointer_cast<Wrapper<PROD> const>(result->sharedConstWrapper());
   }
 }
 #endif