[bugfix] When executing an XQuery that is stored in the database via the REST API only hold a lock on the source document whilst the query is compiled, then release it before executing the query

Closes eXist-db/exist#5916
Closes eXist-db/exist#4867
Closes eXist-db/exist#4022

Author: adamretter

exist-core/src/main/java/org/exist/http/RESTServer.java

@@ -70,6 +70,7 @@
 import org.exist.security.Subject;
 import org.exist.security.internal.RealmImpl;
 import org.exist.source.DBSource;
+import org.exist.source.DbStoreSource;
 import org.exist.source.Source;
 import org.exist.source.StringSource;
 import org.exist.source.URLSource;
@@ -578,7 +579,7 @@ public void doGet(final DBBroker broker, final Txn transaction, final HttpServle
                 try {
                     if (xquery_mime_type.equals(resource.getMediaType())) {
                         // Execute the XQuery
-                        executeXQuery(broker, transaction, resource, request, response,
+                        executeXQuery(broker, transaction, lockedDocument, request, response,
                                 outputProperties, servletPath.toString(), pathInfo);
                     } else if (xproc_mime_type.equals(resource.getMediaType())) {
                         // Execute the XProc
@@ -745,7 +746,7 @@ public void doPost(final DBBroker broker, final Txn transaction, final HttpServl
                     try {
                         if (xquery_mime_type.equals(resource.getMediaType())) {
                             // Execute the XQuery
-                            executeXQuery(broker, transaction, resource, request, response,
+                            executeXQuery(broker, transaction, lockedDocument, request, response,
                                     outputProperties, servletPath.toString(), pathInfo);
                         } else {
                             // Execute the XProc
@@ -1267,7 +1268,7 @@ private boolean checkForXQueryTarget(final DBBroker broker, final Txn transactio
         final Properties outputProperties = new Properties(defaultOutputKeysProperties);
         try {
             // Execute the XQuery
-            executeXQuery(broker, transaction, resource, request, response,
+            executeXQuery(broker, transaction, lockedDocument, request, response,
                     outputProperties, servletPath.toString(), pathInfo);
         } catch (final XPathException e) {
             writeXPathExceptionHtml(response, HttpServletResponse.SC_BAD_REQUEST, UTF_8.name(), null, path.toString(), e);
@@ -1575,41 +1576,64 @@ private void declareExternalAndXQJVariables(final XQueryContext context,
     /**
      * Directly execute an XQuery stored as a binary document in the database.
      *
-     * @throws PermissionDeniedException
+     * NOTE The document will be unlocked after query compilation (only if compilation succeeds).
+     *
+     * @param broker the database broker.
+     * @param transaction the database transaction.
+     * @param lockedQueryDocument the locked document holding the query to be executed.
+     * @param request the HTTP request.
+     * @param response the HTTP response.
+     * @param outputProperties any serialization properties.
+     * @param servletPath the path to the servlet.
+     * @param pathInfo the path.
+     *
+     * @throws XPathException if an error occurs during query compilation or execution.
+     * @throws BadRequestException if the request had a problem.
+     * @throws PermissionDeniedException if the calling user does not have sufficient permissions.
      */
-    private void executeXQuery(final DBBroker broker, final Txn transaction, final DocumentImpl resource,
+    private void executeXQuery(final DBBroker broker, final Txn transaction, final LockedDocument lockedQueryDocument,
             final HttpServletRequest request, final HttpServletResponse response,
             final Properties outputProperties, final String servletPath, final String pathInfo)
             throws XPathException, BadRequestException, PermissionDeniedException {
 
-        final Source source = new DBSource(broker.getBrokerPool(), (BinaryDocument) resource, true);
+        final DocumentImpl resource = lockedQueryDocument.getDocument();
+        final DbStoreSource source = new DBSource(broker.getBrokerPool(), (BinaryDocument) resource, true);
 
         final ConsumerE<XQueryContext, XPathException> setupXqueryContextPreCompilation = xqueryContext -> {
             xqueryContext.setModuleLoadPath(XmldbURI.EMBEDDED_SERVER_URI.append(resource.getCollection().getURI()).toString());
             xqueryContext.setStaticallyKnownDocuments(new XmldbURI[]{ resource.getCollection().getURI() });
         };
 
-        final ConsumerE<XQueryContext, XPathException> setupXqueryContextPreExecution = xqueryContext -> {
-            final HttpRequestWrapper reqw = declareVariables(xqueryContext, null, request, response);
-            reqw.setServletPath(servletPath);
-            reqw.setPathInfo(pathInfo);
-            DebuggeeFactory.checkForDebugRequest(request, xqueryContext);
-        };
+        try {
+            // NOTE(AR) compilationResult will be cleaned up by the try-with-resources wrapped XQueryUtil.execute(...); at present between here and there no exceptions can occur
+            final XQueryUtil.CompilationResult compilationResult = XQueryUtil.compile(broker, source, false, true, setupXqueryContextPreCompilation);
 
-        final BiConsumerE<XQueryContext, XQueryUtil.QueryResult, XPathException> setupXqueryContextPostExecution = (xqueryContext, queryResult) -> {
-            // Pass last modified date to the HTTP response
-            HTTPUtils.addLastModifiedHeader(queryResult.result, xqueryContext);
-        };
+            // NOTE(AR) query is now compiled so we can release the LockedDocument eagerly
+            lockedQueryDocument.close();
 
-        try (final XQueryUtil.QueryResult queryResult = XQueryUtil.query(broker, source, true, null, outputProperties, setupXqueryContextPreCompilation, setupXqueryContextPreExecution, setupXqueryContextPostExecution)) {
 
-            // Special header to indicate whether the compiled query is returned from the cache
-            response.setHeader(XQUERY_CACHED_RESPONSE_HEADER, queryResult.compilationTime == XQueryUtil.CompilationResult.RETRIEVED_CACHED_COMPILED_QUERY ? "true" : "false");
+            final ConsumerE<XQueryContext, XPathException> setupXqueryContextPreExecution = xqueryContext -> {
+                final HttpRequestWrapper reqw = declareVariables(xqueryContext, null, request, response);
+                reqw.setServletPath(servletPath);
+                reqw.setPathInfo(pathInfo);
+                DebuggeeFactory.checkForDebugRequest(request, xqueryContext);
+            };
+
+            final BiConsumerE<XQueryContext, XQueryUtil.QueryResult, XPathException> setupXqueryContextPostExecution = (xqueryContext, queryResult) -> {
+                // Pass last modified date to the HTTP response
+                HTTPUtils.addLastModifiedHeader(queryResult.result, xqueryContext);
+            };
+
+            try (final XQueryUtil.QueryResult queryResult = XQueryUtil.execute(broker, compilationResult, null, outputProperties, setupXqueryContextPreExecution, setupXqueryContextPostExecution)) {
+                // Special header to indicate whether the compiled query is returned from the cache
+                response.setHeader(XQUERY_CACHED_RESPONSE_HEADER, queryResult.compilationTime == XQueryUtil.CompilationResult.RETRIEVED_CACHED_COMPILED_QUERY ? "true" : "false");
+
+                final boolean wrap = "yes".equals(outputProperties.getProperty("_wrap"));
+                writeResults(response, broker, transaction, queryResult, -1, 1, false, outputProperties, wrap);
+            }
 
-            final boolean wrap = "yes".equals(outputProperties.getProperty("_wrap"));
-            writeResults(response, broker, transaction, queryResult, -1, 1, false, outputProperties, wrap);
         } catch (final IOException e) {
-            throw new BadRequestException("Failed to read query from " + resource.getURI(), e);
+            throw new BadRequestException("Failed to read query from: " + source.getDocumentPath(), e);
         }
     }
 

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

@@ -224,7 +224,7 @@ public void stopXQuery() {
      * We try and read the XQuery source document under those conditions, and the read should succeed.
      */
     @Test
-    public void readRunningXQuerySourceDocument() throws InterruptedException, ExecutionException, TimeoutException, XPathException {
+    public void readRunningXQuerySourceDocument() throws InterruptedException, ExecutionException, TimeoutException {
         // Prepare a Callable that will try and read the XQuery's source document
         final Callable<String> readDocumentCallable = () -> {
 
@@ -263,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() throws InterruptedException {
+    public void replaceRunningXQuerySourceDocument() throws InterruptedException, ExecutionException, TimeoutException {
         // Prepare a Callable that will try and replace the XQuery's source document
-        final Callable<Boolean> replaceDocumentCallable = () -> {
+        final Callable<Integer> replaceDocumentCallable = () -> {
 
-            final String replacementQuery = "<replaced/>";
+            try {
+                final String replacementQuery = "<replaced/>";
 
-            final HttpResponse storeResponse = executor.execute(
-                    Request
-                            .Put(docUri)
-                            .addHeader("Content-Type", MediaType.APPLICATION_XQUERY)
-                            .bodyByteArray(replacementQuery.getBytes(UTF_8))
-            ).returnResponse();
+                final HttpResponse storeResponse = executor.execute(
+                        Request
+                                .Put(docUri)
+                                .addHeader("Content-Type", MediaType.APPLICATION_XQUERY)
+                                .bodyByteArray(replacementQuery.getBytes(UTF_8))
+                ).returnResponse();
 
-            // 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 storeResponse.getStatusLine().getStatusCode();
 
-            return SC_CREATED == storeResponse.getStatusLine().getStatusCode();
+            } 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 Future<Integer> replacedDocumentFuture = executorService.submit(replaceDocumentCallable);
+        final Integer replaceDocumentStatus = replacedDocumentFuture.get(TIMEOUT_MS, TimeUnit.MILLISECONDS);
+        assertEquals(SC_CREATED, replaceDocumentStatus.intValue());
+
+        // Check the results of the query
+        final byte[] queryResult = queryResultFuture.get(TIMEOUT_MS, TimeUnit.MILLISECONDS);
+        assertNotNull(queryResult);
+        final String queryResultStr = new String(queryResult, UTF_8);
+        assertTrue(queryResultStr.startsWith("<elapsed-time exit-latch-zero=\"true\">PT"));
+        assertTrue(queryResultStr.endsWith("</elapsed-time>"));
     }
 }