Skip to content

Commit c92dd17

Browse files
michituxmichitux
authored
XCOMMONS-3348: The servlet environment triggers a second load of the xwiki.properties configuration (#1346)
* Trigger the cache initialization in a separate event listener on the ApplicationStartedEvent. This ensures that the cache manager is ready. * Add and adapt unit tests.
1 parent 03547c2 commit c92dd17

File tree

6 files changed

+205
-58
lines changed

6 files changed

+205
-58
lines changed

‎xwiki-commons-core/xwiki-commons-environment/xwiki-commons-environment-servlet/pom.xml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,11 @@
5050
<artifactId>xwiki-commons-cache-api</artifactId>
5151
<version>${project.version}</version>
5252
</dependency>
53+
<dependency>
54+
<groupId>org.xwiki.commons</groupId>
55+
<artifactId>xwiki-commons-observation-api</artifactId>
56+
<version>${project.version}</version>
57+
</dependency>
5358
<dependency>
5459
<groupId>jakarta.servlet</groupId>
5560
<artifactId>jakarta.servlet-api</artifactId>

‎xwiki-commons-core/xwiki-commons-environment/xwiki-commons-environment-servlet/src/main/java/org/xwiki/environment/internal/ServletEnvironment.java‎

Lines changed: 22 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
import java.net.URISyntaxException;
2727
import java.net.URL;
2828
import java.util.Optional;
29-
import java.util.concurrent.atomic.AtomicBoolean;
3029

3130
import javax.inject.Inject;
3231
import javax.inject.Singleton;
@@ -70,35 +69,29 @@ public class ServletEnvironment extends AbstractEnvironment
7069

7170
private Cache<Optional<URL>> resourceURLCache;
7271

73-
private final AtomicBoolean cacheInitializing = new AtomicBoolean();
74-
75-
// SonarQube has no idea about concurrent modifications that make the repeated check for resourceURLCache == null
76-
// necessary.
77-
// See also the comment at the duplicate check that explains why we need to check again inside the "lock".
78-
@SuppressWarnings("java:S2589")
79-
private Cache<Optional<URL>> getResourceURLCache()
72+
/**
73+
* Initialize the cache for resource URLs. This method is called by {@link ServletEnvironmentCacheInitializer} when
74+
* the application is started to ensure that the cache is initialized only once the cache builder is available. The
75+
* cache builder depends on loading resources from this component, so we can't initialize the cache when it is
76+
* first requested.
77+
*
78+
* @since 17.5.0RC1
79+
* @since 17.4.1
80+
* @since 16.10.9
81+
*/
82+
synchronized void initializeCache()
8083
{
81-
// Don't block on cache initialization to avoid loops.
82-
if (this.resourceURLCache == null && this.cacheInitializing.compareAndSet(false, true)) {
84+
if (this.resourceURLCache == null) {
8385
try {
84-
// Compare again inside the "lock" to avoid race conditions. Only after seeing the false value we can
85-
// be sure that we also see the resourceURLCache variable write from the thread that wrote it -
86-
// writing atomics with "set" guarantees sequential consistency.
87-
if (this.resourceURLCache == null) {
88-
// The cache manager can't be injected directly as it depends on this component, so it is
89-
// important to only request it once this component has been initialized.
90-
CacheManager cacheManager = this.componentManager.getInstance(CacheManager.class);
91-
this.resourceURLCache = cacheManager.createNewCache(
92-
new LRUCacheConfiguration("environment.servlet.resourceURLCache", 10000));
93-
}
86+
// The cache manager can't be injected directly as it depends on this component, so it is
87+
// important to only request it once this component has been initialized.
88+
CacheManager cacheManager = this.componentManager.getInstance(CacheManager.class);
89+
this.resourceURLCache = cacheManager.createNewCache(
90+
new LRUCacheConfiguration("environment.servlet.resourceURLCache", 10000));
9491
} catch (Exception e) {
95-
this.logger.error("Failed to initialize resource URL cache.", e);
96-
} finally {
97-
this.cacheInitializing.set(false);
92+
this.logger.error("Failed to initialize the resource URL cache.", e);
9893
}
9994
}
100-
101-
return this.resourceURLCache;
10295
}
10396

10497
/**
@@ -157,10 +150,8 @@ public InputStream getResourceAsStream(String resourceName)
157150
@SuppressWarnings("java:S2789")
158151
public URL getResource(String resourceName)
159152
{
160-
Cache<Optional<URL>> cache = getResourceURLCache();
161-
162-
if (cache != null && this.cacheControl.isCacheReadAllowed()) {
163-
Optional<URL> cachedURL = cache.get(resourceName);
153+
if (this.resourceURLCache != null && this.cacheControl.isCacheReadAllowed()) {
154+
Optional<URL> cachedURL = this.resourceURLCache.get(resourceName);
164155

165156
if (cachedURL != null) {
166157
return cachedURL.orElse(null);
@@ -169,8 +160,8 @@ public URL getResource(String resourceName)
169160

170161
URL url = getResourceInternal(resourceName);
171162

172-
if (cache != null) {
173-
cache.set(resourceName, Optional.ofNullable(url));
163+
if (this.resourceURLCache != null) {
164+
this.resourceURLCache.set(resourceName, Optional.ofNullable(url));
174165
}
175166

176167
return url;

‎xwiki-commons-core/xwiki-commons-environment/xwiki-commons-environment-servlet/src/main/java/org/xwiki/environment/internal/ServletEnvironmentCacheInitializer.java‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
/*
2+
* See the NOTICE file distributed with this work for additional
3+
* information regarding copyright ownership.
4+
*
5+
* This is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU Lesser General Public License as
7+
* published by the Free Software Foundation; either version 2.1 of
8+
* the License, or (at your option) any later version.
9+
*
10+
* This software is distributed in the hope that it will be useful,
11+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
13+
* Lesser General Public License for more details.
14+
*
15+
* You should have received a copy of the GNU Lesser General Public
16+
* License along with this software; if not, write to the Free
17+
* Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
18+
* 02110-1301 USA, or see the FSF site: http://www.fsf.org.
19+
*/
20+
package org.xwiki.environment.internal;
21+
22+
import java.util.List;
23+
24+
import javax.inject.Inject;
25+
import javax.inject.Named;
26+
import javax.inject.Provider;
27+
import javax.inject.Singleton;
28+
29+
import org.xwiki.component.annotation.Component;
30+
import org.xwiki.environment.Environment;
31+
import org.xwiki.observation.AbstractEventListener;
32+
import org.xwiki.observation.event.ApplicationStartedEvent;
33+
import org.xwiki.observation.event.Event;
34+
35+
/**
36+
* Initializes the Servlet Environment Cache when the application starts.
37+
*
38+
* @since 17.5.0RC1
39+
* @since 17.4.1
40+
* @since 16.10.9
41+
* @version $Id$
42+
*/
43+
@Component
44+
@Singleton
45+
@Named(ServletEnvironmentCacheInitializer.NAME)
46+
public class ServletEnvironmentCacheInitializer extends AbstractEventListener
47+
{
48+
/**
49+
* The name of this event listener.
50+
*/
51+
public static final String NAME = "org.xwiki.environment.internal.ServletEnvironmentCacheInitializer";
52+
53+
@Inject
54+
private Provider<Environment> environment;
55+
56+
/**
57+
* Default constructor.
58+
*/
59+
public ServletEnvironmentCacheInitializer()
60+
{
61+
super(NAME, List.of(new ApplicationStartedEvent()));
62+
}
63+
64+
@Override
65+
public void onEvent(Event event, Object source, Object data)
66+
{
67+
if (this.environment.get() instanceof ServletEnvironment servletEnvironment) {
68+
servletEnvironment.initializeCache();
69+
}
70+
}
71+
}

‎xwiki-commons-core/xwiki-commons-environment/xwiki-commons-environment-servlet/src/main/resources/META-INF/components.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
11
org.xwiki.environment.internal.ServletEnvironment
2+
org.xwiki.environment.internal.ServletEnvironmentCacheInitializer

‎xwiki-commons-core/xwiki-commons-environment/xwiki-commons-environment-servlet/src/test/java/org/xwiki/environment/internal/ServletEnvironmentCacheInitializerTest.java‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
/*
2+
* See the NOTICE file distributed with this work for additional
3+
* information regarding copyright ownership.
4+
*
5+
* This is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU Lesser General Public License as
7+
* published by the Free Software Foundation; either version 2.1 of
8+
* the License, or (at your option) any later version.
9+
*
10+
* This software is distributed in the hope that it will be useful,
11+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
13+
* Lesser General Public License for more details.
14+
*
15+
* You should have received a copy of the GNU Lesser General Public
16+
* License along with this software; if not, write to the Free
17+
* Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
18+
* 02110-1301 USA, or see the FSF site: http://www.fsf.org.
19+
*/
20+
package org.xwiki.environment.internal;
21+
22+
import org.junit.jupiter.api.Test;
23+
import org.xwiki.environment.Environment;
24+
import org.xwiki.observation.event.ApplicationStartedEvent;
25+
import org.xwiki.test.junit5.mockito.ComponentTest;
26+
import org.xwiki.test.junit5.mockito.InjectMockComponents;
27+
import org.xwiki.test.junit5.mockito.MockComponent;
28+
29+
import static org.mockito.Mockito.verify;
30+
31+
/**
32+
* Unit tests for {@link ServletEnvironmentCacheInitializer}.
33+
*
34+
* @version $Id$
35+
*/
36+
@ComponentTest
37+
class ServletEnvironmentCacheInitializerTest
38+
{
39+
@InjectMockComponents
40+
private ServletEnvironmentCacheInitializer cacheInitializer;
41+
42+
@MockComponent(classToMock = ServletEnvironment.class)
43+
private Environment environment;
44+
45+
@Test
46+
void onEvent()
47+
{
48+
this.cacheInitializer.onEvent(new ApplicationStartedEvent(), null, null);
49+
verify((ServletEnvironment) this.environment).initializeCache();
50+
}
51+
}

‎xwiki-commons-core/xwiki-commons-environment/xwiki-commons-environment-servlet/src/test/java/org/xwiki/environment/internal/ServletEnvironmentTest.java‎

Lines changed: 55 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -117,20 +117,28 @@ void getResourceWhenServletContextNotSet()
117117
exception.getMessage());
118118
}
119119

120-
@Test
121-
void getResourceOk() throws Exception
120+
@ParameterizedTest
121+
@ValueSource(booleans = { true, false })
122+
void getResourceOk(boolean initializeCache) throws Exception
122123
{
123124
ServletContext servletContext = mock(ServletContext.class);
124125
this.environment.setServletContext(servletContext);
125126
String resourceName = "/test";
126127
when(servletContext.getResource(resourceName)).thenReturn(new URL("file:/path/../test"));
128+
if (initializeCache) {
129+
this.environment.initializeCache();
130+
}
127131

128132
URL expectedURL = new URL("file:/test");
129133
assertEquals(expectedURL, this.environment.getResource(resourceName));
130134
verify(servletContext).getResource(resourceName);
131-
verify(this.cache).set(resourceName, Optional.of(expectedURL));
132-
// As cache control returns false, the cache shouldn't be read.
133-
verify(this.cache, never()).get(any());
135+
if (initializeCache) {
136+
verify(this.cache).set(resourceName, Optional.of(expectedURL));
137+
// As cache control returns false, the cache shouldn't be read.
138+
verify(this.cache, never()).get(any());
139+
} else {
140+
verifyNoInteractions(this.cache);
141+
}
134142
}
135143

136144
@Test
@@ -145,37 +153,55 @@ void getResourceAsStreamOk() throws Exception
145153
verifyNoInteractions(this.cache);
146154
}
147155

148-
@Test
149-
void getResourceNotExisting() throws Exception
156+
@ParameterizedTest
157+
@ValueSource(booleans = { true, false })
158+
void getResourceNotExisting(boolean initializeCache)
150159
{
151-
ServletContext servletContext = mock(ServletContext.class);
160+
ServletContext servletContext = mock();
152161
this.environment.setServletContext(servletContext);
153162
String resourceName = "unknown resource";
163+
if (initializeCache) {
164+
this.environment.initializeCache();
165+
}
166+
154167
assertNull(this.environment.getResource(resourceName));
155-
verify(this.cache).set(resourceName, Optional.empty());
156-
// As cache control returns false, the cache shouldn't be read.
157-
verify(this.cache, never()).get(any());
168+
if (initializeCache) {
169+
verify(this.cache).set(resourceName, Optional.empty());
170+
// As cache control returns false, the cache shouldn't be read.
171+
verify(this.cache, never()).get(any());
172+
} else {
173+
verifyNoInteractions(this.cache);
174+
}
158175
}
159176

160-
@Test
161-
void getResourceWhenMalformedURLException() throws Exception
177+
@ParameterizedTest
178+
@ValueSource(booleans = { true, false })
179+
void getResourceWhenMalformedURLException(boolean initializeCache) throws Exception
162180
{
163181
ServletContext servletContext = mock(ServletContext.class);
164182
String resourceName = "bad resource";
165183
when(servletContext.getResource(resourceName)).thenThrow(new MalformedURLException("invalid url"));
166184
this.environment.setServletContext(servletContext);
185+
if (initializeCache) {
186+
this.environment.initializeCache();
187+
}
167188
assertNull(this.environment.getResource(resourceName));
168189
assertEquals("Error getting resource [bad resource] because of invalid path format. Reason: [invalid url]",
169190
logCapture.getMessage(0));
170-
verify(this.cache).set(resourceName, Optional.empty());
171-
// As cache control returns false, the cache shouldn't be read.
172-
verify(this.cache, never()).get(any());
191+
if (initializeCache) {
192+
verify(this.cache).set(resourceName, Optional.empty());
193+
// As cache control returns false, the cache shouldn't be read.
194+
verify(this.cache, never()).get(any());
195+
} else {
196+
verifyNoInteractions(this.cache);
197+
}
173198
}
174199

175200
@Test
176201
void getResourceNotExistingFromCache()
177202
{
178203
String resourceName = "unknown resource";
204+
this.environment.initializeCache();
179205
when(this.cacheControl.isCacheReadAllowed()).thenReturn(true);
180206
when(this.cache.get(resourceName)).thenReturn(Optional.empty());
181207
assertNull(this.environment.getResource(resourceName));
@@ -188,6 +214,7 @@ void getResourceExistingFromCache()
188214
{
189215
String resourceName = "known resource";
190216
URL expectedURL = mock(URL.class);
217+
this.environment.initializeCache();
191218
when(this.cacheControl.isCacheReadAllowed()).thenReturn(true);
192219
when(this.cache.get(resourceName)).thenReturn(Optional.of(expectedURL));
193220
assertEquals(expectedURL, this.environment.getResource(resourceName));
@@ -230,8 +257,11 @@ void getResourceWithRecursiveCallInCacheInitialization(boolean cached) throws Ex
230257
ExecutorService executor = Executors.newFixedThreadPool(1);
231258

232259
try {
233-
CompletableFuture<URL> outerCall =
234-
CompletableFuture.supplyAsync(() -> this.environment.getResource(resourceName), executor);
260+
CompletableFuture<Void> initializeCall =
261+
CompletableFuture.supplyAsync(() -> {
262+
this.environment.initializeCache();
263+
return null;
264+
}, executor);
235265

236266
// Wait for the background thread to arrive in the cache creation call (but don't wait forever just to be
237267
// safe).
@@ -244,18 +274,16 @@ void getResourceWithRecursiveCallInCacheInitialization(boolean cached) throws Ex
244274
// Unblock the cache creation call.
245275
blockCreateCacheFuture.complete(null);
246276

247-
// Ensure that the blocked call now got the value, too. Again, don't wait forever just in case.
248-
URL actualOuterURL = outerCall.get(20, TimeUnit.SECONDS);
249-
if (cached) {
250-
assertEquals(cachedURL, actualOuterURL);
251-
} else {
252-
assertEquals(expectedURL, actualOuterURL);
253-
}
277+
// Ensure that the blocked call now completed. Again, don't wait forever just in case.
278+
initializeCall.get(20, TimeUnit.SECONDS);
254279

255-
// Assert that the recursive call also got the URL.
280+
// Assert that the recursive call got the URL.
256281
assertEquals(expectedURL, recursiveURL.getValue());
257282

258-
// Only the outer call should have accessed the cache. If the cache didn't return the value, it should
283+
// Assert that we now get the cached URL.
284+
assertEquals(cached ? cachedURL : expectedURL, this.environment.getResource(resourceName));
285+
286+
// Only the last call should have accessed the cache. If the cache didn't return the value, it should
259287
// have been stored.
260288
verify(this.cache).get(resourceName);
261289
if (cached) {

0 commit comments

Comments
 (0)