Merge pull request #3622 from adamretter/hotfix/document-use-path-locks

Fix NPE and Deadlock when Path Locks are used for Documents

Author: dizzzz

exist-core/src/main/java/org/exist/storage/NativeBroker.java

@@ -2209,7 +2209,8 @@ public LockedDocument getXMLResource(XmldbURI fileName, final LockMode lockMode)
         //TODO : resolve URIs !
         final XmldbURI collUri = fileName.removeLastSegment();
         final XmldbURI docUri = fileName.lastSegment();
-        try(final Collection collection = openCollection(collUri, LockMode.READ_LOCK)) {
+        final LockMode collectionLockMode = lockManager.relativeCollectionLockMode(LockMode.READ_LOCK, lockMode);
+        try(final Collection collection = openCollection(collUri, collectionLockMode)) {
             if (collection == null) {
                 LOG.debug("Collection '{}' not found!", collUri);
                 return null;

exist-core/src/main/java/org/exist/storage/lock/LockManager.java

@@ -576,7 +576,12 @@ public ManagedDocumentLock acquireDocumentWriteLock(final XmldbURI documentPath)
      * @return true if a WRITE_LOCK is held
      */
     public boolean isDocumentLockedForWrite(final XmldbURI documentPath) {
-        final MultiLock existingLock = getDocumentLock(documentPath.toString());
+        final MultiLock existingLock;
+        if (!usePathLocksForDocuments) {
+            existingLock = getDocumentLock(documentPath.toString());
+        } else {
+            existingLock = getPathLock(documentPath.toString());
+        }
         return existingLock.getWriteLockCount() > 0;
     }
 
@@ -588,10 +593,57 @@ public boolean isDocumentLockedForWrite(final XmldbURI documentPath) {
      * @return true if a READ_LOCK is held
      */
     public boolean isDocumentLockedForRead(final XmldbURI documentPath) {
-        final MultiLock existingLock = getDocumentLock(documentPath.toString());
+        final MultiLock existingLock;
+        if (!usePathLocksForDocuments) {
+            existingLock = getDocumentLock(documentPath.toString());
+        } else {
+            existingLock = getPathLock(documentPath.toString());
+        }
         return existingLock.getReadLockCount() > 0;
     }
 
+    /**
+     * Returns the LockMode that should be used for accessing
+     * a Collection when that access is just for the purposes
+     * or accessing a document(s) within the Collection.
+     *
+     * When Path Locks are enabled for Documents, both Collections and
+     * Documents share the same lock domain, a path based tree hierarchy.
+     * With a shared lock-hierarchy, if we were to READ_LOCK a Collection
+     * and then request a WRITE_LOCK for a Document in that Collection we
+     * would reach a dead-lock situation. To avoid this, if this method is
+     * called like
+     * {@code relativeCollectionLockMode(LockMode.READ_LOCK, LockMode.WRITE_LOCK)}
+     * it will return a WRITE_LOCK. That is to say that to aid dealock-avoidance,
+     * this function may return a stricter locking mode than the {@code desiredCollectionLockMode}.
+     *
+     * When Path Locks are disabled (the default) for Documents, Collection and Documents
+     * have independent locking domains. In this case this function will always return
+     * the {@code desiredCollectionLockMode}.
+     *
+     * @param desiredCollectionLockMode The desired lock mode for the Collection.
+     * @param documentLockMode The lock mode that will be used for subsequent document operations in the Collection.
+     *
+     * @return The lock mode that should be used for accessing the Collection.
+     */
+    public Lock.LockMode relativeCollectionLockMode(final Lock.LockMode desiredCollectionLockMode,
+            final Lock.LockMode documentLockMode) {
+        if (!usePathLocksForDocuments) {
+            return desiredCollectionLockMode;
+
+        } else {
+            switch (documentLockMode) {
+                case NO_LOCK:
+                case INTENTION_READ:
+                case READ_LOCK:
+                    return Lock.LockMode.READ_LOCK;
+
+                default:
+                    return Lock.LockMode.WRITE_LOCK;
+            }
+        }
+    }
+
     /**
      * Retrieves a lock for a {@link org.exist.storage.dom.DOMFile}
      *

exist-core/src/main/java/org/exist/xmlrpc/RpcConnection.java

@@ -275,9 +275,11 @@ protected LockedDocumentMap beginProtected(final DBBroker broker, final Map<Stri
         do {
             MutableDocumentSet docs = null;
             final LockedDocumentMap lockedDocuments = new LockedDocumentMap();
-            try(final Collection coll = broker.openCollection(XmldbURI.createInternal(protectColl), LockMode.READ_LOCK)) {
+            final LockMode documentLockMode = LockMode.WRITE_LOCK;
+            final LockMode collectionLockMode = broker.getBrokerPool().getLockManager().relativeCollectionLockMode(LockMode.READ_LOCK, documentLockMode);
+            try (final Collection coll = broker.openCollection(XmldbURI.createInternal(protectColl), collectionLockMode)) {
                 docs = new DefaultDocumentSet();
-                coll.allDocs(broker, docs, true, lockedDocuments, LockMode.WRITE_LOCK);
+                coll.allDocs(broker, docs, true, lockedDocuments, documentLockMode);
                 return lockedDocuments;
             } catch (final LockException e) {
                 LOG.warn("Deadlock detected. Starting over again. Docs: {}; locked: {}. Cause: {}", docs.getDocumentCount(), lockedDocuments.size(), e.getMessage());

exist-core/src/test/java/org/exist/collections/CollectionRemovalTest.java

@@ -134,7 +134,7 @@ private void removeCollection(final String user, final String password, final Xm
     private void retrieveDoc(final XmldbURI uri) throws EXistException, PermissionDeniedException, SAXException, LockException {
         final BrokerPool pool = existEmbeddedServer.getBrokerPool();
         try(final DBBroker broker = pool.get(Optional.of(pool.getSecurityManager().getSystemSubject()));
-                final Collection test = broker.openCollection(uri, LockMode.WRITE_LOCK)) {
+                final Collection test = broker.openCollection(uri, LockMode.READ_LOCK)) {
             assertNotNull(test);
 
             try(final LockedDocument lockedDoc = test.getDocumentWithLock(broker, XmldbURI.createInternal("document.xml"), LockMode.READ_LOCK)) {

exist-core/src/test/java/org/exist/collections/triggers/HistoryTriggerTest.java

@@ -132,7 +132,7 @@ public void storeAndOverwriteByCopy() throws EXistException, PermissionDeniedExc
             storeInTestCollection(transaction, broker, testDoc2Name, testDoc2Content);
 
             // overwrite the first document by copying the second over it (and make sure we don't get a StackOverflow exception)
-            try(final Collection testCollection = broker.openCollection(TEST_COLLECTION_URI, Lock.LockMode.WRITE_LOCK);) {
+            try(final Collection testCollection = broker.openCollection(TEST_COLLECTION_URI, Lock.LockMode.WRITE_LOCK)) {
                 assertNotNull(testCollection);
 
                 try(final LockedDocument lockedDoc2 = testCollection.getDocumentWithLock(broker, testDoc2Name, Lock.LockMode.READ_LOCK)) {

exist-core/src/test/java/org/exist/config/TwoDatabasesTest.java

@@ -143,7 +143,7 @@ private Collection storeBin(final DBBroker broker, final Txn txn, String suffix)
         return top;
     }
 
-    private boolean getBin(final DBBroker broker, final String suffix) throws PermissionDeniedException, IOException, LockException {
+    private boolean getBin(final DBBroker broker, final String suffix) throws PermissionDeniedException, IOException {
         BinaryDocument binDoc = null;
         try(final Collection top = broker.openCollection(XmldbURI.create("xmldb:exist:///db"), LockMode.READ_LOCK)) {
             binDoc = (BinaryDocument) top.getDocument(broker, XmldbURI.create("bin"));

exist-core/src/test/java/org/exist/util/XMLReaderSecurityTest.java

@@ -159,7 +159,7 @@ public void expandExternalEntities() throws EXistException, IOException, Permiss
 
             try (final Collection testCollection = broker.openCollection(TEST_COLLECTION, Lock.LockMode.READ_LOCK)) {
 
-                try (final LockedDocument testDoc = testCollection.getDocumentWithLock(broker, docName, Lock.LockMode.READ_LOCK);){
+                try (final LockedDocument testDoc = testCollection.getDocumentWithLock(broker, docName, Lock.LockMode.READ_LOCK)) {
 
                     // release the collection lock early inline with asymmetrical locking
                     testCollection.close();
@@ -192,7 +192,7 @@ public void cannotExpandExternalEntitiesWhenDisabled() throws EXistException, IO
         try (final DBBroker broker = brokerPool.get(Optional.of(brokerPool.getSecurityManager().getSystemSubject()));
              final Txn transaction = brokerPool.getTransactionManager().beginTransaction()) {
 
-            try (final Collection testCollection = broker.openCollection(TEST_COLLECTION, Lock.LockMode.WRITE_LOCK);){
+            try (final Collection testCollection = broker.openCollection(TEST_COLLECTION, Lock.LockMode.WRITE_LOCK)) {
 
                 //debugReader("cannotExpandExternalEntitiesWhenDisabled", broker, testCollection);
 

exist-core/src/test/java/org/exist/xquery/functions/securitymanager/PermissionsFunctionChownTest.java

@@ -1602,7 +1602,7 @@ private static void assertDocumentSetUidSetGid(final Subject execAsUser, final X
     private static void assertCollectionSetUidSetGid(final Subject execAsUser, final XmldbURI uri, final boolean isSet) throws EXistException, PermissionDeniedException {
         final BrokerPool pool = existWebServer.getBrokerPool();
         try (final DBBroker broker = pool.get(Optional.of(execAsUser))) {
-            try (final Collection col = broker.openCollection(uri, Lock.LockMode.READ_LOCK);) {
+            try (final Collection col = broker.openCollection(uri, Lock.LockMode.READ_LOCK)) {
 
                 if (isSet) {
                     assertTrue(col.getPermissions().isSetUid());