Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Feb 13, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a9ab0fe
XCOMMONS-3258: Introduce a cache for ServletEnvironment#getResource
michitux Feb 12, 2025
28a50a7
XCOMMONS-3258: Introduce a cache for ServletEnvironment#getResource
michitux Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions xwiki-commons-core/xwiki-commons-environment/xwiki-commons-environment-servlet/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
64 changes: 64 additions & 0 deletions ...-environment-servlet/src/main/java/org/xwiki/environment/internal/ServletEnvironment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -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()}
*/
Expand Down Expand Up @@ -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 {
Expand Down
170 changes: 157 additions & 13 deletions ...ironment-servlet/src/test/java/org/xwiki/environment/internal/ServletEnvironmentTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
Loading