XCOMMONS-3258: Introduce a cache for ServletEnvironment#getResource (#1234)

michitux · web-flow · commit 51f405d60271 · 2025-02-13T11:18:02.000+01:00
* Introduce a dynamically initialized cache to avoid issues with
  recursive component initialization calls.
* Add test cases to confirm that the cache works as expected and that
  cache initialization can do recursive calls for getting resources.
diff --git a/xwiki-commons-core/xwiki-commons-environment/xwiki-commons-environment-servlet/pom.xml b/xwiki-commons-core/xwiki-commons-environment/xwiki-commons-environment-servlet/pom.xml
@@ -45,6 +45,11 @@
       <artifactId>xwiki-commons-jakartabridge-servlet</artifactId>
       <version>${project.version}</version>
     </dependency>
+    <dependency>
+      <groupId>org.xwiki.commons</groupId>
+      <artifactId>xwiki-commons-cache-api</artifactId>
+      <version>${project.version}</version>
+    </dependency>
     <dependency>
       <groupId>jakarta.servlet</groupId>
       <artifactId>jakarta.servlet-api</artifactId>
diff --git a/xwiki-commons-core/xwiki-commons-environment/xwiki-commons-environment-servlet/src/main/java/org/xwiki/environment/internal/ServletEnvironment.java b/xwiki-commons-core/xwiki-commons-environment/xwiki-commons-environment-servlet/src/main/java/org/xwiki/environment/internal/ServletEnvironment.java
@@ -25,13 +25,21 @@
 import java.net.MalformedURLException;
 import java.net.URISyntaxException;
 import java.net.URL;
+import java.util.Optional;
+import java.util.concurrent.atomic.AtomicBoolean;
 
+import javax.inject.Inject;
 import javax.inject.Singleton;
 
 import jakarta.servlet.ServletContext;
 
 import org.apache.commons.lang3.exception.ExceptionUtils;
+import org.xwiki.cache.Cache;
+import org.xwiki.cache.CacheControl;
+import org.xwiki.cache.CacheManager;
+import org.xwiki.cache.config.LRUCacheConfiguration;
 import org.xwiki.component.annotation.Component;
+import org.xwiki.component.manager.ComponentManager;
 import org.xwiki.jakartabridge.servlet.JakartaServletBridge;
 
 /**
@@ -54,6 +62,41 @@ public class ServletEnvironment extends AbstractEnvironment
      */
     private javax.servlet.ServletContext javaxServletContext;
 
+    @Inject
+    private ComponentManager componentManager;
+
+    @Inject
+    private CacheControl cacheControl;
+
+    private Cache<Optional<URL>> resourceURLCache;
+
+    private final AtomicBoolean cacheInitializing = new AtomicBoolean();
+
+    private Cache<Optional<URL>> getResourceURLCache()
+    {
+        // Don't block on cache initialization to avoid loops.
+        if (this.resourceURLCache == null && this.cacheInitializing.compareAndSet(false, true)) {
+            try {
+                // Compare again inside the "lock" to avoid race conditions. Only after seeing the false value we can
+                // be sure that we also see the resourceURLCache variable write from the thread that wrote it -
+                // writing atomics with "set" guarantees sequential consistency.
+                if (this.resourceURLCache == null) {
+                    // The cache manager can't be injected directly as it depends on this component, so it is
+                    // important to only request it once this component has been initialized.
+                    CacheManager cacheManager = this.componentManager.getInstance(CacheManager.class);
+                    this.resourceURLCache = cacheManager.createNewCache(
+                        new LRUCacheConfiguration("environment.servlet.resourceURLCache", 10000));
+                }
+            } catch (Exception e) {
+                this.logger.error("Failed to initialize resource URL cache.", e);
+            } finally {
+                this.cacheInitializing.set(false);
+            }
+        }
+
+        return this.resourceURLCache;
+    }
+
     /**
      * @param servletContext see {@link #getServletContext()}
      */
@@ -107,6 +150,27 @@ public InputStream getResourceAsStream(String resourceName)
 
     @Override
     public URL getResource(String resourceName)
+    {
+        Cache<Optional<URL>> cache = getResourceURLCache();
+
+        if (cache != null && this.cacheControl.isCacheReadAllowed()) {
+            Optional<URL> cachedURL = cache.get(resourceName);
+
+            if (cachedURL != null) {
+                return cachedURL.orElse(null);
+            }
+        }
+
+        URL url = getResourceInternal(resourceName);
+
+        if (cache != null) {
+            cache.set(resourceName, Optional.ofNullable(url));
+        }
+
+        return url;
+    }
+
+    private URL getResourceInternal(String resourceName)
     {
         URL url;
         try {
diff --git a/xwiki-commons-core/xwiki-commons-environment/xwiki-commons-environment-servlet/src/test/java/org/xwiki/environment/internal/ServletEnvironmentTest.java b/xwiki-commons-core/xwiki-commons-environment/xwiki-commons-environment-servlet/src/test/java/org/xwiki/environment/internal/ServletEnvironmentTest.java
@@ -22,37 +22,71 @@
 import java.io.File;
 import java.net.MalformedURLException;
 import java.net.URL;
+import java.util.Optional;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
 
 import jakarta.servlet.ServletContext;
 
 import org.apache.commons.io.FileUtils;
+import org.apache.commons.lang3.mutable.Mutable;
+import org.apache.commons.lang3.mutable.MutableObject;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.mockito.Mock;
 import org.slf4j.Logger;
-import org.xwiki.component.embed.EmbeddableComponentManager;
+import org.xwiki.cache.Cache;
+import org.xwiki.cache.CacheControl;
+import org.xwiki.cache.CacheManager;
 import org.xwiki.component.util.ReflectionUtils;
-import org.xwiki.environment.Environment;
 import org.xwiki.test.junit5.LogCaptureExtension;
+import org.xwiki.test.junit5.mockito.ComponentTest;
+import org.xwiki.test.junit5.mockito.InjectMockComponents;
+import org.xwiki.test.junit5.mockito.MockComponent;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.mockito.Mockito.*;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
+import static org.mockito.Mockito.when;
 
 /**
  * Unit tests for {@link ServletEnvironment}.
  *
  * @version $Id$
  * @since 3.5M1
  */
+@SuppressWarnings({ "checkstyle:ClassFanOutComplexity", "checkstyle:MultipleStringLiterals" })
+@ComponentTest
 class ServletEnvironmentTest
 {
     private File servletTmpDir;
 
     private File systemTmpDir;
 
+    @MockComponent
+    private CacheManager cacheManager;
+
+    @Mock
+    private Cache<Optional<URL>> cache;
+
+    @MockComponent
+    private CacheControl cacheControl;
+
+    @InjectMockComponents
     private ServletEnvironment environment;
 
     @RegisterExtension
@@ -64,9 +98,7 @@ public void setUp() throws Exception
         this.servletTmpDir = new File(System.getProperty("java.io.tmpdir"), "ServletEnvironmentTest-tmpDir");
         this.systemTmpDir = new File(System.getProperty("java.io.tmpdir"), "xwiki-temp");
 
-        EmbeddableComponentManager ecm = new EmbeddableComponentManager();
-        ecm.initialize(getClass().getClassLoader());
-        this.environment = ecm.getInstance(Environment.class);
+        doReturn(this.cache).when(this.cacheManager).createNewCache(any());
     }
 
     @AfterEach
@@ -90,10 +122,15 @@ void getResourceOk() throws Exception
     {
         ServletContext servletContext = mock(ServletContext.class);
         this.environment.setServletContext(servletContext);
-        when(servletContext.getResource("/test")).thenReturn(new URL("file:/path/../test"));
-
-        assertEquals(new URL("file:/test"), this.environment.getResource("/test"));
-        verify(servletContext).getResource("/test");
+        String resourceName = "/test";
+        when(servletContext.getResource(resourceName)).thenReturn(new URL("file:/path/../test"));
+
+        URL expectedURL = new URL("file:/test");
+        assertEquals(expectedURL, this.environment.getResource(resourceName));
+        verify(servletContext).getResource(resourceName);
+        verify(this.cache).set(resourceName, Optional.of(expectedURL));
+        // As cache control returns false, the cache shouldn't be read.
+        verify(this.cache, never()).get(any());
     }
 
     @Test
@@ -104,25 +141,132 @@ void getResourceAsStreamOk() throws Exception
         this.environment.getResourceAsStream("/test");
 
         verify(servletContext).getResourceAsStream("/test");
+        // No cache for streams.
+        verifyNoInteractions(this.cache);
     }
 
     @Test
     void getResourceNotExisting() throws Exception
     {
         ServletContext servletContext = mock(ServletContext.class);
         this.environment.setServletContext(servletContext);
-        assertNull(this.environment.getResource("unknown resource"));
+        String resourceName = "unknown resource";
+        assertNull(this.environment.getResource(resourceName));
+        verify(this.cache).set(resourceName, Optional.empty());
+        // As cache control returns false, the cache shouldn't be read.
+        verify(this.cache, never()).get(any());
     }
 
     @Test
     void getResourceWhenMalformedURLException() throws Exception
     {
         ServletContext servletContext = mock(ServletContext.class);
-        when(servletContext.getResource("bad resource")).thenThrow(new MalformedURLException("invalid url"));
+        String resourceName = "bad resource";
+        when(servletContext.getResource(resourceName)).thenThrow(new MalformedURLException("invalid url"));
         this.environment.setServletContext(servletContext);
-        assertNull(this.environment.getResource("bad resource"));
+        assertNull(this.environment.getResource(resourceName));
         assertEquals("Error getting resource [bad resource] because of invalid path format. Reason: [invalid url]",
             logCapture.getMessage(0));
+        verify(this.cache).set(resourceName, Optional.empty());
+        // As cache control returns false, the cache shouldn't be read.
+        verify(this.cache, never()).get(any());
+    }
+
+    @Test
+    void getResourceNotExistingFromCache()
+    {
+        String resourceName = "unknown resource";
+        when(this.cacheControl.isCacheReadAllowed()).thenReturn(true);
+        when(this.cache.get(resourceName)).thenReturn(Optional.empty());
+        assertNull(this.environment.getResource(resourceName));
+        verify(this.cache).get(resourceName);
+        verify(this.cache, never()).set(any(), any());
+    }
+
+    @Test
+    void getResourceExistingFromCache()
+    {
+        String resourceName = "known resource";
+        URL expectedURL = mock(URL.class);
+        when(this.cacheControl.isCacheReadAllowed()).thenReturn(true);
+        when(this.cache.get(resourceName)).thenReturn(Optional.of(expectedURL));
+        assertEquals(expectedURL, this.environment.getResource(resourceName));
+        verify(this.cache).get(resourceName);
+        verify(this.cache, never()).set(any(), any());
+    }
+
+    @ParameterizedTest
+    @ValueSource(booleans = { true, false })
+    void getResourceWithRecursiveCallInCacheInitialization(boolean cached) throws Exception
+    {
+        ServletContext servletContext = mock(ServletContext.class);
+        String resourceName = "/resource";
+        when(servletContext.getResource(resourceName)).thenReturn(new URL("file:/path/../resource"));
+        this.environment.setServletContext(servletContext);
+
+        when(this.cacheControl.isCacheReadAllowed()).thenReturn(true);
+
+        URL cachedURL = mock();
+        if (cached) {
+            when(this.cache.get(resourceName)).thenReturn(Optional.of(cachedURL));
+        }
+
+        // Two futures to synchronize the background thread that creates the cache.
+        CompletableFuture<Void> arrivedInCreateCacheFuture = new CompletableFuture<>();
+        CompletableFuture<Void> blockCreateCacheFuture = new CompletableFuture<>();
+
+        Mutable<URL> recursiveURL = new MutableObject<>();
+
+        doAnswer(invocationOnMock -> {
+            // Signal that the background thread arrived in the cache creation call.
+            arrivedInCreateCacheFuture.complete(null);
+            recursiveURL.setValue(this.environment.getResource(resourceName));
+            // Wait until the main thread unblocks the cache creation (but don't wait forever just to be safe).
+            blockCreateCacheFuture.get(20, TimeUnit.SECONDS);
+            return this.cache;
+        }).when(this.cacheManager).createNewCache(any());
+
+        // Launch a background thread so we can test blocking in the creation of the cache.
+        ExecutorService executor = Executors.newFixedThreadPool(1);
+
+        try {
+            CompletableFuture<URL> outerCall =
+                CompletableFuture.supplyAsync(() -> this.environment.getResource(resourceName), executor);
+
+            // Wait for the background thread to arrive in the cache creation call (but don't wait forever just to be
+            // safe).
+            arrivedInCreateCacheFuture.get(20, TimeUnit.SECONDS);
+
+            URL expectedURL = new URL("file:/resource");
+            // Ensure that the cache creation doesn't block getting the resource URL.
+            assertEquals(expectedURL, this.environment.getResource(resourceName));
+
+            // Unblock the cache creation call.
+            blockCreateCacheFuture.complete(null);
+
+            // Ensure that the blocked call now got the value, too. Again, don't wait forever just in case.
+            URL actualOuterURL = outerCall.get(20, TimeUnit.SECONDS);
+            if (cached) {
+                assertEquals(cachedURL, actualOuterURL);
+            } else {
+                assertEquals(expectedURL, actualOuterURL);
+            }
+
+            // Assert that the recursive call also got the URL.
+            assertEquals(expectedURL, recursiveURL.getValue());
+
+            // Only the outer call should have accessed the cache. If the cache didn't return the value, it should
+            // have been stored.
+            verify(this.cache).get(resourceName);
+            if (cached) {
+                verify(this.cache, never()).set(any(), any());
+            } else {
+                verify(this.cache).set(resourceName, Optional.of(expectedURL));
+            }
+            verify(servletContext, times(cached ? 2 : 3)).getResource(resourceName);
+        } finally {
+            executor.shutdownNow();
+        }
     }
 
     @Test
