[bugfix] When executing an XQuery that is stored in the database via the XML:DB and RPC APIs only hold a lock on the source document whilst the query is compiled, then release it before executing the query

Closes eXist-db/exist#4022

Author: adamretter

exist-core/src/main/java/org/exist/xmldb/LocalXPathQueryService.java

@@ -182,7 +182,7 @@ public EXistResourceSet query(final XMLResource res, final String query, final S
     }
 
     private EXistResourceSet doQuery(final DBBroker broker, final Txn transaction, final String query, final XmldbURI[] docs, final Sequence contextSet, final String sortExpr) throws XMLDBException {
-        final Either<XPathException, CompiledExpression> maybeExpr = compileAndCheck(broker, transaction, query);
+        final Either<XPathException, LocalCompiledExpression> maybeExpr = compileAndCheck(broker, transaction, query);
         if(maybeExpr.isLeft()) {
             final XPathException e = maybeExpr.left().get();
             throw new XMLDBException(ErrorCodes.VENDOR_ERROR, e.getMessage(), e);
@@ -269,6 +269,9 @@ public EXistResourceSet execute(final Source source) throws XMLDBException {
     @Override
     public EXistResourceSet executeStoredQuery(final String uri) throws XMLDBException {
         return withDb((broker, transaction) -> {
+
+            final Either<XPathException, LocalCompiledExpression> maybeExpr;
+
             try (final LockedDocument lockedResource = broker.getXMLResource(new XmldbURI(uri), LockMode.READ_LOCK)) {
                 if (lockedResource == null) {
                     throw new XMLDBException(ErrorCodes.INVALID_URI, "No stored XQuery exists at: " + uri);
@@ -279,8 +282,15 @@ public EXistResourceSet executeStoredQuery(final String uri) throws XMLDBExcepti
                 }
 
                 final Source dbSource = new DBSource(broker.getBrokerPool(), (BinaryDocument) resource, false);
+                maybeExpr = compileAndCheck(broker, transaction, dbSource);
+            }
 
-                return execute(broker, transaction, dbSource);
+            // NOTE(AR) query is now compiled so we can release the LockedDocument eagerly above
+            if (maybeExpr.isLeft()) {
+                final XPathException e = maybeExpr.left().get();
+                throw new XMLDBException(ErrorCodes.VENDOR_ERROR, e.getMessage(), e);
+            } else {
+                return execute(broker, transaction, null, null, maybeExpr.right().get(), null);
             }
         });
     }
@@ -341,7 +351,7 @@ private EXistResourceSet execute(final DBBroker broker, final Txn transaction, f
     @Override
     public CompiledExpression compile(final String query) throws XMLDBException {
         return withDb((broker, transaction) -> {
-            final Either<XPathException, CompiledExpression> maybeExpr = compileAndCheck(broker, transaction, query);
+            final Either<XPathException, LocalCompiledExpression> maybeExpr = compileAndCheck(broker, transaction, query);
             if(maybeExpr.isLeft()) {
                 final XPathException e = maybeExpr.left().get();
                 throw new XMLDBException(ErrorCodes.VENDOR_ERROR, e.getMessage(), e);
@@ -353,18 +363,20 @@ public CompiledExpression compile(final String query) throws XMLDBException {
 
     @Override
     public CompiledExpression compileAndCheck(final String query) throws XMLDBException, XPathException {
-    	final Either<XPathException, CompiledExpression> result = withDb((broker, transaction) -> compileAndCheck(broker, transaction, query));
+    	final Either<XPathException, LocalCompiledExpression> result = withDb((broker, transaction) -> compileAndCheck(broker, transaction, query));
         if(result.isLeft()) {
             throw result.left().get();
         } else {
             return result.right().get();
         }
     }
 
-    private Either<XPathException, CompiledExpression> compileAndCheck(final DBBroker broker, final Txn transaction, final String query) throws XMLDBException {
-
+    private Either<XPathException, LocalCompiledExpression> compileAndCheck(final DBBroker broker, final Txn transaction, final String query) throws XMLDBException {
         final Source source = new StringSource(query);
+        return compileAndCheck(broker, transaction, source);
+    }
 
+    private Either<XPathException, LocalCompiledExpression> compileAndCheck(final DBBroker broker, final Txn transaction, final Source source) throws XMLDBException {
         final ConsumerE<XQueryContext, XPathException> preCompilationContext = xqueryContext -> setupContext(source, xqueryContext);
 
         try {
@@ -373,7 +385,7 @@ private Either<XPathException, CompiledExpression> compileAndCheck(final DBBroke
                 LOG.debug("Compilation took {}ms", compilationResult.compilationTime);
             }
 
-            final CompiledExpression compiledExpression = new LocalCompiledExpression(compilationResult);
+            final LocalCompiledExpression compiledExpression = new LocalCompiledExpression(compilationResult);
             return Either.Right(compiledExpression);
         } catch (final PermissionDeniedException e) {
             throw new XMLDBException(ErrorCodes.PERMISSION_DENIED, e.getMessage(), e);

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

@@ -151,6 +151,8 @@
 import static org.exist.xmldb.EXistXPathQueryService.BEGIN_PROTECTED_MAX_LOCKING_RETRIES;
 import static java.nio.file.StandardOpenOption.*;
 
+// TODO(AR) this class leaks resources from the XQueryPool and XQueryContext not being cleaned up correctly, switch to XQueryUtil for compilation/execution
+
 /**
  * This class implements the actual methods defined by
  * {@link org.exist.xmlrpc.RpcAPI}.
@@ -2159,22 +2161,31 @@ public Map<String, Object> executeT(final String pathToQuery, final Map<String,
 
         final Optional<String> sortBy = Optional.ofNullable(parameters.get(RpcAPI.SORT_EXPR)).map(Object::toString);
 
-        return this.<Map<String, Object>>readDocument(XmldbURI.createInternal(pathToQuery)).apply((document, broker, transaction) -> {
-            final BinaryDocument xquery = (BinaryDocument) document;
-            if (xquery.getResourceType() != DocumentImpl.BINARY_FILE) {
-                throw new EXistException("Document " + pathToQuery + " is not a binary resource");
-            }
+        return withDb((broker, transaction) -> {
 
-            if (!xquery.getPermissions().validate(user, Permission.READ | Permission.EXECUTE)) {
-                throw new PermissionDeniedException("Insufficient privileges to access resource");
-            }
+            final CompiledXQuery compiledXquery = this.<CompiledXQuery>readDocument(broker, transaction, XmldbURI.createInternal(pathToQuery)).apply((document, broker1, transaction1) -> {
+                if (document.getResourceType() != DocumentImpl.BINARY_FILE) {
+                    throw new EXistException("Document " + pathToQuery + " is not a binary resource");
+                }
 
-            final Source source = new DBSource(broker.getBrokerPool(), xquery, true);
+                if (!document.getPermissions().validate(user, Permission.READ | Permission.EXECUTE)) {
+                    throw new PermissionDeniedException("Insufficient privileges to access resource");
+                }
 
+                final Source source = new DBSource(broker1.getBrokerPool(), (BinaryDocument) document, true);
+                try {
+                    return this.<CompiledXQuery>compileQuery(broker1, transaction1, source, parameters).apply(cx -> (CompiledXQuery) cx);
+                } catch (final XPathException e) {
+                    throw new EXistException(e);
+                }
+            });
+
+            // NOTE(AR) query is now compiled so we can release the readDocument lock eagerly above
             try {
-                final Map<String, Object> rpcResponse = this.<Map<String, Object>>compileQuery(broker, transaction, source, parameters)
-                        .apply(compiledQuery -> queryResultToTypedRpcResponse(startTime, getXdmSerializationOptions(compiledQuery.getContext()), doQuery(broker, compiledQuery, null, parameters), sortBy));
-                return rpcResponse;
+                final QueryResult queryResult = doQuery(broker, compiledXquery, null, parameters);
+
+                final Properties xdmSerializationOptions = getXdmSerializationOptions(compiledXquery.getContext());
+                return queryResultToTypedRpcResponse(startTime, xdmSerializationOptions, queryResult, sortBy);
             } catch (final XPathException e) {
                 throw new EXistException(e);
             }

exist-core/src/test/java/org/exist/xquery/RunningXQueryXmlDbTest.java

@@ -73,7 +73,6 @@
 import static org.exist.util.PropertiesBuilder.propertiesBuilder;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 
 /**
@@ -264,36 +263,47 @@ public void readRunningXQuerySourceDocument() throws InterruptedException, Execu
 
     /**
      * When this test is run, the /db/running-xquery-test/latched-query.xq XQuery
-     * is already executing (see {@link #runXQuery()}) in a separate thread that holds a READ_LOCK on its Source document.
+     * is already executing (see {@link #runXQuery()}) in a separate thread,
+     * that should have released the READ_LOCK on its Source document after query compilation.
      *
-     * We try and replace the XQuery source document under those conditions, which should not be possible
-     * as writing to the document would require a WRITE_LOCK, but a READ_LOCK is already held.
+     * We try and replace the XQuery source document under those conditions, which should be possible
+     * as no lock is held on the source document after compilation.
      */
     @Test
-    public void replaceRunningXQuerySourceDocument() {
+    public void replaceRunningXQuerySourceDocument() throws ExecutionException, InterruptedException, TimeoutException {
         // Prepare a Callable that will try and replace the XQuery's source document
         final Callable<Boolean> replaceDocumentCallable = () -> {
 
-            final String replacementQuery = "<replaced/>";
+            try {
+                final String replacementQuery = "<replaced/>";
 
-            try (final Collection testCollection = DatabaseManager.getCollection(getBaseUri() + TEST_COLLECTION_URI.getCollectionPath(), TestUtils.ADMIN_DB_USER, TestUtils.ADMIN_DB_PWD);
-                 final EXistResource queryResource = (EXistResource) testCollection.getResource(LATCHED_QUERY_URI.getCollectionPath())) {
-                queryResource.setMediaType(MediaType.APPLICATION_XQUERY);
-                queryResource.setContent(replacementQuery.getBytes(UTF_8));
-                testCollection.storeResource(queryResource);
-            }
+                try (final Collection testCollection = DatabaseManager.getCollection(getBaseUri() + TEST_COLLECTION_URI.getCollectionPath(), TestUtils.ADMIN_DB_USER, TestUtils.ADMIN_DB_PWD);
+                     final EXistResource queryResource = (EXistResource) testCollection.getResource(LATCHED_QUERY_URI.getCollectionPath())) {
+                    queryResource.setMediaType(MediaType.APPLICATION_XQUERY);
+                    queryResource.setContent(replacementQuery.getBytes(UTF_8));
+                    testCollection.storeResource(queryResource);
+                }
 
-            // NOTE(AR) It should not have been possible to get to this point as we should not be able to acquire a WRITE_LOCK lock on the document (above) when calling broker.storeDocument, as the query thread holds a READ_LOCK on the document
+                return true;
 
-            return true;
+            } finally {
+                // Instruct the running XQuery to finish
+                @Nullable final CountDownLatch exitLatch = LatchWrappersSingleton.INSTANCE.queryExitLatchRef.get();
+                assertNotNull("exitLatch should not be null at this point", exitLatch);
+                exitLatch.countDown();
+            }
         };
 
         // Run the replaceDocumentCallable from a separate thread so that we don't block our test
         final Future<Boolean> replacedDocumentFuture = executorService.submit(replaceDocumentCallable);
-        assertThrows(
-                "We should not have been able to replace the document as that requires this thread to obtain a WRITE_LOCK on the document, but the query thread already holds a READ_LOCK on the document",
-                TimeoutException.class, () ->
-                        replacedDocumentFuture.get(TIMEOUT_MS, TimeUnit.MILLISECONDS)
-        );
+        final Boolean replaceDocumentStatus = replacedDocumentFuture.get(TIMEOUT_MS, TimeUnit.MILLISECONDS);
+        assertTrue(replaceDocumentStatus);
+
+        // Check the results of the query
+        // Check the results of the query
+        final String queryResult = queryResultFuture.get(TIMEOUT_MS, TimeUnit.MILLISECONDS);
+        assertNotNull(queryResult);
+        assertTrue(queryResult.startsWith("<elapsed-time exit-latch-zero=\"true\">PT"));
+        assertTrue(queryResult.endsWith("</elapsed-time>"));
     }
 }