It is enough to hold the read lock.

5 files changed, 50 insertions, 48 deletions

diff --git a/searchcore/src/apps/tests/persistenceconformance_test.cpp b/searchcore/src/apps/tests/persistenceconformance_test.cpp
index ba0d2047b88..4056e49e37c 100644
--- a/searchcore/src/apps/tests/persistenceconformance_test.cpp
+++ b/searchcore/src/apps/tests/persistenceconformance_test.cpp
@@ -331,7 +331,7 @@ public:
             LOG(info, "putHandler(%s)", itr->first.toString().c_str());
             IPersistenceHandler::SP proxy(
                     new PersistenceHandlerProxy(itr->second));
-            putHandler(itr->second->getBucketSpace(), itr->first, proxy);
+            putHandler(getWLock(), itr->second->getBucketSpace(), itr->first, proxy);
         }
     }
 
@@ -343,7 +343,7 @@ public:
         const DocumentDBMap &docDbs = _docDbRepo->getDocDbs();
         for (DocumentDBMap::const_iterator itr = docDbs.begin();
              itr != docDbs.end(); ++itr) {
-            IPersistenceHandler::SP proxy(removeHandler(itr->second->getBucketSpace(), itr->first));
+            IPersistenceHandler::SP proxy(removeHandler(getWLock(), itr->second->getBucketSpace(), itr->first));
         }
     }
 
diff --git a/searchcore/src/tests/proton/persistenceengine/persistenceengine_test.cpp b/searchcore/src/tests/proton/persistenceengine/persistenceengine_test.cpp
index 569b36a425d..19d9d41c3e4 100644
--- a/searchcore/src/tests/proton/persistenceengine/persistenceengine_test.cpp
+++ b/searchcore/src/tests/proton/persistenceengine/persistenceengine_test.cpp
@@ -404,8 +404,8 @@ struct SimpleFixture {
           engine(_owner, _writeFilter, -1, false),
           hset()
     {
-        engine.putHandler(makeBucketSpace(), DocTypeName(doc1->getType()), hset.phandler1);
-        engine.putHandler(bucketSpace2, DocTypeName(doc2->getType()), hset.phandler2);
+        engine.putHandler(engine.getWLock(), makeBucketSpace(), DocTypeName(doc1->getType()), hset.phandler1);
+        engine.putHandler(engine.getWLock(), bucketSpace2, DocTypeName(doc2->getType()), hset.phandler2);
     }
     SimpleFixture()
         : SimpleFixture(makeBucketSpace())
diff --git a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp
index 1f862b07048..9d40cbae633 100644
--- a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp
+++ b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.cpp
@@ -40,7 +40,7 @@ protected:
     std::mutex               _lock;
     vespalib::CountDownLatch _latch;
 public:
-    ResultHandlerBase(uint32_t waitCnt);
+    explicit ResultHandlerBase(uint32_t waitCnt);
     ~ResultHandlerBase();
     void await() { _latch.await(); }
 };
@@ -55,11 +55,11 @@ class GenericResultHandler : public ResultHandlerBase, public IGenericResultHand
 private:
     Result _result;
 public:
-    GenericResultHandler(uint32_t waitCnt) :
+    explicit GenericResultHandler(uint32_t waitCnt) :
         ResultHandlerBase(waitCnt),
         _result()
     { }
-    ~GenericResultHandler();
+    ~GenericResultHandler() override;
     void handle(const Result &result) override {
         if (result.hasError()) {
             std::lock_guard<std::mutex> guard(_lock);
@@ -109,7 +109,7 @@ class SynchronizedBucketIdListResultHandler : public ResultHandlerBase,
                                               public BucketIdListResultHandler
 {
 public:
-    SynchronizedBucketIdListResultHandler(uint32_t waitCnt)
+    explicit SynchronizedBucketIdListResultHandler(uint32_t waitCnt)
         : ResultHandlerBase(waitCnt),
           BucketIdListResultHandler()
     { }
@@ -164,16 +164,20 @@ BucketInfoResultHandler::~BucketInfoResultHandler() = default;
 }
 
 PersistenceEngine::HandlerSnapshot::UP
-PersistenceEngine::getHandlerSnapshot() const
+PersistenceEngine::getHandlerSnapshot(const WriteGuard &) const
 {
-    std::lock_guard<std::mutex> guard(_lock);
     return _handlers.getHandlerSnapshot();
 }
 
 PersistenceEngine::HandlerSnapshot::UP
-PersistenceEngine::getHandlerSnapshot(document::BucketSpace bucketSpace) const
+PersistenceEngine::getHandlerSnapshot(const ReadGuard &, document::BucketSpace bucketSpace) const
+{
+    return _handlers.getHandlerSnapshot(bucketSpace);
+}
+
+PersistenceEngine::HandlerSnapshot::UP
+PersistenceEngine::getHandlerSnapshot(const WriteGuard &, document::BucketSpace bucketSpace) const
 {
-    std::lock_guard<std::mutex> guard(_lock);
     return _handlers.getHandlerSnapshot(bucketSpace);
 }
 
@@ -202,27 +206,23 @@ PersistenceEngine::~PersistenceEngine()
 
 
 IPersistenceHandler::SP
-PersistenceEngine::putHandler(document::BucketSpace bucketSpace, const DocTypeName &docType,
-                              const IPersistenceHandler::SP &handler)
+PersistenceEngine::putHandler(const WriteGuard &, document::BucketSpace bucketSpace, const DocTypeName &docType,const IPersistenceHandler::SP &handler)
 {
-    std::lock_guard<std::mutex> guard(_lock);
     return _handlers.putHandler(bucketSpace, docType, handler);
 }
 
 
 IPersistenceHandler::SP
-PersistenceEngine::getHandler(document::BucketSpace bucketSpace, const DocTypeName &docType) const
+PersistenceEngine::getHandler(const ReadGuard &, document::BucketSpace bucketSpace, const DocTypeName &docType) const
 {
-    std::lock_guard<std::mutex> guard(_lock);
     return _handlers.getHandler(bucketSpace, docType);
 }
 
 
 IPersistenceHandler::SP
-PersistenceEngine::removeHandler(document::BucketSpace bucketSpace, const DocTypeName &docType)
+PersistenceEngine::removeHandler(const WriteGuard &, document::BucketSpace bucketSpace, const DocTypeName &docType)
 {
     // TODO: Grab bucket list and treat them as modified
-    std::lock_guard<std::mutex> guard(_lock);
     return _handlers.removeHandler(bucketSpace, docType);
 }
 
@@ -232,7 +232,7 @@ PersistenceEngine::initialize()
 {
     std::unique_lock<std::shared_timed_mutex> wguard(getWLock());
     LOG(debug, "Begin initializing persistence handlers");
-    HandlerSnapshot::UP snap = getHandlerSnapshot();
+    HandlerSnapshot::UP snap = getHandlerSnapshot(wguard);
     for (; snap->handlers().valid(); snap->handlers().next()) {
         IPersistenceHandler *handler = snap->handlers().get();
         handler->initialize();
@@ -260,7 +260,7 @@ PersistenceEngine::listBuckets(BucketSpace bucketSpace, PartitionId id) const
         BucketIdListResult::List emptyList;
         return BucketIdListResult(emptyList);
     }
-    HandlerSnapshot::UP snap = getHandlerSnapshot(bucketSpace);
+    HandlerSnapshot::UP snap = getHandlerSnapshot(rguard, bucketSpace);
     BucketIdListResultHandler resultHandler;
     for (; snap->handlers().valid(); snap->handlers().next()) {
         IPersistenceHandler *handler = snap->handlers().get();
@@ -275,7 +275,7 @@ PersistenceEngine::setClusterState(BucketSpace bucketSpace, const ClusterState &
 {
     std::shared_lock<std::shared_timed_mutex> rguard(_rwMutex);
     saveClusterState(bucketSpace, calc);
-    HandlerSnapshot::UP snap = getHandlerSnapshot(bucketSpace);
+    HandlerSnapshot::UP snap = getHandlerSnapshot(rguard, bucketSpace);
     GenericResultHandler resultHandler(snap->size());
     for (; snap->handlers().valid(); snap->handlers().next()) {
         IPersistenceHandler *handler = snap->handlers().get();
@@ -292,7 +292,7 @@ PersistenceEngine::setActiveState(const Bucket& bucket,
                                   storage::spi::BucketInfo::ActiveState newState)
 {
     std::shared_lock<std::shared_timed_mutex> rguard(_rwMutex);
-    HandlerSnapshot::UP snap = getHandlerSnapshot(bucket.getBucketSpace());
+    HandlerSnapshot::UP snap = getHandlerSnapshot(rguard, bucket.getBucketSpace());
     GenericResultHandler resultHandler(snap->size());
     for (; snap->handlers().valid(); snap->handlers().next()) {
         IPersistenceHandler *handler = snap->handlers().get();
@@ -309,7 +309,7 @@ PersistenceEngine::getBucketInfo(const Bucket& b) const
     // Runs in SPI thread.
     // No handover to write threads in persistence handlers.
     std::shared_lock<std::shared_timed_mutex> rguard(_rwMutex);
-    HandlerSnapshot::UP snap = getHandlerSnapshot(b.getBucketSpace());
+    HandlerSnapshot::UP snap = getHandlerSnapshot(rguard, b.getBucketSpace());
     BucketInfoResultHandler resultHandler;
     for (; snap->handlers().valid(); snap->handlers().next()) {
         IPersistenceHandler *handler = snap->handlers().get();
@@ -338,7 +338,7 @@ PersistenceEngine::put(const Bucket& b, Timestamp t, const document::Document::S
         return Result(Result::ErrorType::PERMANENT_ERROR,
                       make_string("Old id scheme not supported in elastic mode (%s)", doc->getId().toString().c_str()));
     }
-    IPersistenceHandler::SP handler = getHandler(b.getBucketSpace(), docType);
+    IPersistenceHandler::SP handler = getHandler(rguard, b.getBucketSpace(), docType);
     if (!handler) {
         return Result(Result::ErrorType::PERMANENT_ERROR,
                       make_string("No handler for document type '%s'", docType.toString().c_str()));
@@ -360,7 +360,7 @@ PersistenceEngine::remove(const Bucket& b, Timestamp t, const DocumentId& did, C
                             make_string("Old id scheme not supported in elastic mode (%s)", did.toString().c_str()));
     }
     DocTypeName docType(did.getDocType());
-    IPersistenceHandler::SP handler = getHandler(b.getBucketSpace(), docType);
+    IPersistenceHandler::SP handler = getHandler(rguard, b.getBucketSpace(), docType);
     if (!handler) {
         return RemoveResult(Result::ErrorType::PERMANENT_ERROR,
                             make_string("No handler for document type '%s'", docType.toString().c_str()));
@@ -414,7 +414,7 @@ PersistenceEngine::update(const Bucket& b, Timestamp t, const DocumentUpdate::SP
         return UpdateResult(Result::ErrorType::PERMANENT_ERROR,
                             make_string("Update operation rejected due to bad id (%s, %s)", upd->getId().toString().c_str(), docType.getName().c_str()));
     }
-    IPersistenceHandler::SP handler = getHandler(b.getBucketSpace(), docType);
+    IPersistenceHandler::SP handler = getHandler(rguard, b.getBucketSpace(), docType);
 
     if (handler) {
         TransportLatch latch(1);
@@ -432,7 +432,7 @@ PersistenceEngine::GetResult
 PersistenceEngine::get(const Bucket& b, const document::FieldSet& fields, const DocumentId& did, Context& context) const
 {
     std::shared_lock<std::shared_timed_mutex> rguard(_rwMutex);
-    HandlerSnapshot::UP snapshot = getHandlerSnapshot(b.getBucketSpace());
+    HandlerSnapshot::UP snapshot = getHandlerSnapshot(rguard, b.getBucketSpace());
 
     for (PersistenceHandlerSequence & handlers = snapshot->handlers(); handlers.valid(); handlers.next()) {
         BucketGuard::UP bucket_guard = handlers.get()->lockBucket(b);
@@ -462,7 +462,7 @@ PersistenceEngine::createIterator(const Bucket &bucket, const document::FieldSet
                                   IncludedVersions versions, Context & context)
 {
     std::shared_lock<std::shared_timed_mutex> rguard(_rwMutex);
-    HandlerSnapshot::UP snapshot = getHandlerSnapshot(bucket.getBucketSpace());
+    HandlerSnapshot::UP snapshot = getHandlerSnapshot(rguard, bucket.getBucketSpace());
 
     auto entry = std::make_unique<IteratorEntry>(context.getReadConsistency(), bucket, fields, selection,
                                                  versions, _defaultSerializedSize, _ignoreMaxBytes);
@@ -541,7 +541,7 @@ PersistenceEngine::createBucket(const Bucket &b, Context &)
 {
     std::shared_lock<std::shared_timed_mutex> rguard(_rwMutex);
     LOG(spam, "createBucket(%s)", b.toString().c_str());
-    HandlerSnapshot::UP snap = getHandlerSnapshot(b.getBucketSpace());
+    HandlerSnapshot::UP snap = getHandlerSnapshot(rguard, b.getBucketSpace());
     TransportLatch latch(snap->size());
     for (; snap->handlers().valid(); snap->handlers().next()) {
         IPersistenceHandler *handler = snap->handlers().get();
@@ -557,7 +557,7 @@ PersistenceEngine::deleteBucket(const Bucket& b, Context&)
 {
     std::shared_lock<std::shared_timed_mutex> rguard(_rwMutex);
     LOG(spam, "deleteBucket(%s)", b.toString().c_str());
-    HandlerSnapshot::UP snap = getHandlerSnapshot(b.getBucketSpace());
+    HandlerSnapshot::UP snap = getHandlerSnapshot(rguard, b.getBucketSpace());
     TransportLatch latch(snap->size());
     for (; snap->handlers().valid(); snap->handlers().next()) {
         IPersistenceHandler *handler = snap->handlers().get();
@@ -578,7 +578,7 @@ PersistenceEngine::getModifiedBuckets(BucketSpace bucketSpace) const
         std::lock_guard<std::mutex> guard(_lock);
         extraModifiedBuckets.swap(_extraModifiedBuckets[bucketSpace]);
     }
-    HandlerSnapshot::UP snap = getHandlerSnapshot(bucketSpace);
+    HandlerSnapshot::UP snap = getHandlerSnapshot(rguard, bucketSpace);
     SynchronizedBucketIdListResultHandler resultHandler(snap->size() + extraModifiedBuckets.size());
     for (; snap->handlers().valid(); snap->handlers().next()) {
         IPersistenceHandler *handler = snap->handlers().get();
@@ -599,7 +599,7 @@ PersistenceEngine::split(const Bucket& source, const Bucket& target1, const Buck
     LOG(spam, "split(%s, %s, %s)", source.toString().c_str(), target1.toString().c_str(), target2.toString().c_str());
     assert(source.getBucketSpace() == target1.getBucketSpace());
     assert(source.getBucketSpace() == target2.getBucketSpace());
-    HandlerSnapshot::UP snap = getHandlerSnapshot(source.getBucketSpace());
+    HandlerSnapshot::UP snap = getHandlerSnapshot(rguard, source.getBucketSpace());
     TransportLatch latch(snap->size());
     for (; snap->handlers().valid(); snap->handlers().next()) {
         IPersistenceHandler *handler = snap->handlers().get();
@@ -617,7 +617,7 @@ PersistenceEngine::join(const Bucket& source1, const Bucket& source2, const Buck
     LOG(spam, "join(%s, %s, %s)", source1.toString().c_str(), source2.toString().c_str(), target.toString().c_str());
     assert(source1.getBucketSpace() == target.getBucketSpace());
     assert(source2.getBucketSpace() == target.getBucketSpace());
-    HandlerSnapshot::UP snap = getHandlerSnapshot(target.getBucketSpace());
+    HandlerSnapshot::UP snap = getHandlerSnapshot(rguard, target.getBucketSpace());
     TransportLatch latch(snap->size());
     for (; snap->handlers().valid(); snap->handlers().next()) {
         IPersistenceHandler *handler = snap->handlers().get();
@@ -722,10 +722,9 @@ public:
 };
 
 void
-PersistenceEngine::populateInitialBucketDB(BucketSpace bucketSpace,
-                                           IPersistenceHandler &targetHandler)
+PersistenceEngine::populateInitialBucketDB(const WriteGuard & guard, BucketSpace bucketSpace, IPersistenceHandler &targetHandler)
 {
-    HandlerSnapshot::UP snap = getHandlerSnapshot(bucketSpace);
+    HandlerSnapshot::UP snap = getHandlerSnapshot(guard, bucketSpace);
     
     size_t snapSize(snap->size());
     size_t flawed = 0;
diff --git a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.h b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.h
index a6c696d08fb..3f2cf92acb7 100644
--- a/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.h
+++ b/searchcore/src/vespa/searchcore/proton/persistenceengine/persistenceengine.h
@@ -75,9 +75,13 @@ private:
     mutable ExtraModifiedBuckets            _extraModifiedBuckets;
     mutable std::shared_timed_mutex         _rwMutex;
 
-    IPersistenceHandler::SP getHandler(document::BucketSpace bucketSpace, const DocTypeName &docType) const;
-    HandlerSnapshot::UP getHandlerSnapshot() const;
-    HandlerSnapshot::UP getHandlerSnapshot(document::BucketSpace bucketSpace) const;
+    using ReadGuard = std::shared_lock<std::shared_timed_mutex>;
+    using WriteGuard = std::unique_lock<std::shared_timed_mutex>;
+
+    IPersistenceHandler::SP getHandler(const ReadGuard & guard, document::BucketSpace bucketSpace, const DocTypeName &docType) const;
+    HandlerSnapshot::UP getHandlerSnapshot(const WriteGuard & guard) const;
+    HandlerSnapshot::UP getHandlerSnapshot(const ReadGuard & guard, document::BucketSpace bucketSpace) const;
+    HandlerSnapshot::UP getHandlerSnapshot(const WriteGuard & guard, document::BucketSpace bucketSpace) const;
 
     void saveClusterState(BucketSpace bucketSpace, const ClusterState &calc);
     ClusterState::SP savedClusterState(BucketSpace bucketSpace) const;
@@ -89,9 +93,8 @@ public:
                       ssize_t defaultSerializedSize, bool ignoreMaxBytes);
     ~PersistenceEngine() override;
 
-    IPersistenceHandler::SP putHandler(document::BucketSpace bucketSpace, const DocTypeName &docType,
-                                       const IPersistenceHandler::SP &handler);
-    IPersistenceHandler::SP removeHandler(document::BucketSpace bucketSpace, const DocTypeName &docType);
+    IPersistenceHandler::SP putHandler(const WriteGuard &, document::BucketSpace bucketSpace, const DocTypeName &docType, const IPersistenceHandler::SP &handler);
+    IPersistenceHandler::SP removeHandler(const WriteGuard &, document::BucketSpace bucketSpace, const DocTypeName &docType);
 
     // Implements PersistenceProvider
     Result initialize() override;
@@ -121,8 +124,8 @@ public:
     void destroyIterators();
     void propagateSavedClusterState(BucketSpace bucketSpace, IPersistenceHandler &handler);
     void grabExtraModifiedBuckets(BucketSpace bucketSpace, IPersistenceHandler &handler);
-    void populateInitialBucketDB(BucketSpace bucketSpace, IPersistenceHandler &targetHandler);
-    std::unique_lock<std::shared_timed_mutex> getWLock() const;
+    void populateInitialBucketDB(const WriteGuard & guard, BucketSpace bucketSpace, IPersistenceHandler &targetHandler);
+    WriteGuard getWLock() const;
 };
 
 }
diff --git a/searchcore/src/vespa/searchcore/proton/server/proton.cpp b/searchcore/src/vespa/searchcore/proton/server/proton.cpp
index 4daf3e895af..28de4dff917 100644
--- a/searchcore/src/vespa/searchcore/proton/server/proton.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/proton.cpp
@@ -594,10 +594,10 @@ Proton::addDocumentDB(const document::DocumentType &docType,
         auto persistenceHandler = std::make_shared<PersistenceHandlerProxy>(ret);
         if (!_isInitializing) {
             _persistenceEngine->propagateSavedClusterState(bucketSpace, *persistenceHandler);
-            _persistenceEngine->populateInitialBucketDB(bucketSpace, *persistenceHandler);
+            _persistenceEngine->populateInitialBucketDB(persistenceWGuard, bucketSpace, *persistenceHandler);
         }
         // TODO: Fix race with new cluster state setting.
-        _persistenceEngine->putHandler(bucketSpace, docTypeName, persistenceHandler);
+        _persistenceEngine->putHandler(persistenceWGuard, bucketSpace, docTypeName, persistenceHandler);
     }
     auto searchHandler = std::make_shared<SearchHandlerProxy>(ret);
     _summaryEngine->putSearchHandler(docTypeName, searchHandler);
@@ -629,7 +629,7 @@ Proton::removeDocumentDB(const DocTypeName &docTypeName)
             // Not allowed to get to service layer to call pause().
             std::unique_lock<std::shared_timed_mutex> persistenceWguard(_persistenceEngine->getWLock());
             IPersistenceHandler::SP oldHandler;
-            oldHandler = _persistenceEngine->removeHandler(old->getBucketSpace(), docTypeName);
+            oldHandler = _persistenceEngine->removeHandler(persistenceWguard, old->getBucketSpace(), docTypeName);
             if (_initComplete && oldHandler) {
                 // TODO: Fix race with bucket db modifying ops.
                 _persistenceEngine->grabExtraModifiedBuckets(old->getBucketSpace(), *oldHandler);