This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/struts.git
The following commit(s) were added to refs/heads/main by this push:
new 05ad78a06 WW-5690 perf(dispatcher): load the dev-mode error template
on first use (#1864)
05ad78a06 is described below
commit 05ad78a0691d80d87f96f22e60a1616f5a9100cb
Author: Lukasz Lenart <[email protected]>
AuthorDate: Mon Aug 24 14:30:36 2026 +0200
WW-5690 perf(dispatcher): load the dev-mode error template on first use
(#1864)
* WW-5690 perf(dispatcher): load the dev-mode error template on first use
DefaultDispatcherErrorHandler.init() built a FreeMarker configuration and
loaded /org/apache/struts2/dispatcher/error.ftl on every startup, including
in production where the problem report is never rendered - handleError()
delegates to the container's error page unless devMode is on.
Defer the load to the first dev-mode error instead. Two threads racing there
may both load the template, which is harmless: FreeMarker caches templates
in
its own configuration, and that is cheaper than locking a path taken once
per
application.
One behavioural change: a missing or unparsable error.ftl used to fail the
application at boot. It now surfaces on the first dev-mode error, where the
existing catch in handleErrorInDevMode() degrades to sendError() with the
cause. A dev-only template should not stop a production application
starting.
This removes the startup cost only; the FreeMarker dependency itself stays.
Co-Authored-By: Claude Opus 5 <[email protected]>
* WW-5690 refactor(dispatcher): synchronise the lazy template load
Sonar flags the volatile Template field (java:S3077): volatile publishes the
reference safely but guarantees nothing about the object's own state, and
Template extends Configurable, so it is not strictly immutable.
The lock-free version was not worth defending anyway. It was justified as
avoiding a lock on a path taken once per application, but that path only
runs
when devMode is on and a request has already failed - it is never hot. And
FreemarkerManager.getConfiguration is itself synchronized, so this path was
already taking a lock.
A plain field behind a synchronized getter is simpler, obviously correct,
and
costs nothing here.
Co-Authored-By: Claude Opus 5 <[email protected]>
---------
Co-authored-by: Claude Opus 5 <[email protected]>
---
.../dispatcher/DefaultDispatcherErrorHandler.java | 30 ++++++++---
.../DefaultDispatcherErrorHandlerTest.java | 59 ++++++++++++++++++++++
2 files changed, 82 insertions(+), 7 deletions(-)
diff --git
a/core/src/main/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandler.java
b/core/src/main/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandler.java
index d64345854..779ca7cd1 100644
---
a/core/src/main/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandler.java
+++
b/core/src/main/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandler.java
@@ -30,7 +30,6 @@ import org.apache.commons.lang3.BooleanUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.StrutsConstants;
-import org.apache.struts2.StrutsException;
import org.apache.struts2.views.freemarker.FreemarkerManager;
import java.io.IOException;
@@ -48,8 +47,11 @@ public class DefaultDispatcherErrorHandler implements
DispatcherErrorHandler {
private static final Logger LOG =
LogManager.getLogger(DefaultDispatcherErrorHandler.class);
+ private static final String ERROR_TEMPLATE =
"/org/apache/struts2/dispatcher/error.ftl";
+
private FreemarkerManager freemarkerManager;
private boolean devMode;
+ private ServletContext servletContext;
private Template template;
@Inject
@@ -63,12 +65,26 @@ public class DefaultDispatcherErrorHandler implements
DispatcherErrorHandler {
}
public void init(ServletContext ctx) {
- try {
- freemarker.template.Configuration config =
freemarkerManager.getConfiguration(ctx);
- template =
config.getTemplate("/org/apache/struts2/dispatcher/error.ftl");
- } catch (IOException e) {
- throw new StrutsException(e);
+ this.servletContext = ctx;
+ }
+
+ /**
+ * Loads the problem report template on first use rather than at startup:
it is only ever
+ * rendered in devMode, so a production application should never build a
FreeMarker
+ * configuration on its behalf.
+ * <p>
+ * Synchronised rather than lock-free: this runs only when devMode is on
and a request has
+ * already failed, and {@link
FreemarkerManager#getConfiguration(ServletContext)} is itself
+ * synchronised, so the lock costs nothing that this path was not paying
already.
+ *
+ * @return the problem report template
+ * @throws IOException if the template cannot be loaded
+ */
+ protected synchronized Template getTemplate() throws IOException {
+ if (template == null) {
+ template =
freemarkerManager.getConfiguration(servletContext).getTemplate(ERROR_TEMPLATE);
}
+ return template;
}
public void handleError(HttpServletRequest request, HttpServletResponse
response, int code, Exception e) {
@@ -115,7 +131,7 @@ public class DefaultDispatcherErrorHandler implements
DispatcherErrorHandler {
} while ((cur = cur.getCause()) != null);
Writer writer = new StringWriter();
- template.process(createReportData(e, chain), writer);
+ getTemplate().process(createReportData(e, chain), writer);
response.setContentType("text/html");
response.getWriter().write(writer.toString());
diff --git
a/core/src/test/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandlerTest.java
b/core/src/test/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandlerTest.java
index 6d44c7dac..f1f521047 100644
---
a/core/src/test/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandlerTest.java
+++
b/core/src/test/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandlerTest.java
@@ -18,10 +18,14 @@
*/
package org.apache.struts2.dispatcher;
+import freemarker.template.Configuration;
import java.io.IOException;
+import java.io.PrintWriter;
+import java.io.StringWriter;
import java.util.Collections;
import org.apache.struts2.StrutsInternalTestCase;
+import jakarta.servlet.ServletContext;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import org.apache.struts2.views.freemarker.FreemarkerManager;
@@ -130,6 +134,61 @@ public class DefaultDispatcherErrorHandlerTest extends
StrutsInternalTestCase {
defaultDispatcherErrorHandler.handleError(requestMock, responseMock,
HttpServletResponse.SC_INTERNAL_SERVER_ERROR, fakeException);
}
+ /**
+ * The dev-mode problem report is the only thing that needs FreeMarker
here, so booting the
+ * application must not build a FreeMarker configuration just to have the
template ready.
+ */
+ public void testInitDoesNotLoadErrorTemplate() {
+ RecordingFreemarkerManager freemarkerManager =
createFreemarkerManager();
+ DefaultDispatcherErrorHandler defaultDispatcherErrorHandler = new
DefaultDispatcherErrorHandler();
+ defaultDispatcherErrorHandler.setDevMode("true");
+ defaultDispatcherErrorHandler.setFreemarkerManager(freemarkerManager);
+
+ defaultDispatcherErrorHandler.init(dispatcher.servletContext);
+
+ assertFalse("init() must not touch FreeMarker",
freemarkerManager.configurationRequested);
+ }
+
+ /**
+ * The deferred load must still happen, otherwise the problem report
silently stops rendering.
+ */
+ public void testErrorTemplateLoadedOnFirstDevModeError() throws
IOException {
+ RecordingFreemarkerManager freemarkerManager =
createFreemarkerManager();
+ DefaultDispatcherErrorHandler defaultDispatcherErrorHandler = new
DefaultDispatcherErrorHandler();
+ defaultDispatcherErrorHandler.setDevMode("true");
+ defaultDispatcherErrorHandler.setFreemarkerManager(freemarkerManager);
+ defaultDispatcherErrorHandler.init(dispatcher.servletContext);
+ Exception fakeException = new Exception("Fake Exception, devMode
true");
+ responseMock.setContentType("text/html");
+ expectLastCall();
+ expect(responseMock.getWriter()).andStubReturn(new PrintWriter(new
StringWriter()));
+ replay(responseMock);
+
+ defaultDispatcherErrorHandler.handleError(requestMock, responseMock,
HttpServletResponse.SC_INTERNAL_SERVER_ERROR, fakeException);
+
+ assertTrue("problem report must load the template on first use",
freemarkerManager.configurationRequested);
+ }
+
+ private RecordingFreemarkerManager createFreemarkerManager() {
+ RecordingFreemarkerManager freemarkerManager = new
RecordingFreemarkerManager();
+ container.inject(freemarkerManager);
+ return freemarkerManager;
+ }
+
+ /**
+ * Records whether the FreeMarker configuration was ever asked for, while
still delegating so the
+ * template really renders.
+ */
+ private static class RecordingFreemarkerManager extends FreemarkerManager {
+ private boolean configurationRequested;
+
+ @Override
+ public Configuration getConfiguration(ServletContext servletContext) {
+ configurationRequested = true;
+ return super.getConfiguration(servletContext);
+ }
+ }
+
protected void setUp() {
requestMock = (HttpServletRequest)
createMock(HttpServletRequest.class);
responseMock = (HttpServletResponse)
createMock(HttpServletResponse.class);
