From 7497fe0f5d72934c0d3af0b069783545112d89c9 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 24 Aug 2026 11:56:00 +0200 Subject: [PATCH 1/2] 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 --- .../DefaultDispatcherErrorHandler.java | 34 ++++++++--- .../DefaultDispatcherErrorHandlerTest.java | 59 +++++++++++++++++++ 2 files changed, 85 insertions(+), 8 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 d643458541..290db77539 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.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,9 +47,12 @@ 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 Template template; + private ServletContext servletContext; + private volatile Template template; @Inject public void setFreemarkerManager(FreemarkerManager freemarkerManager) { @@ -63,12 +65,28 @@ public void setDevMode(String devMode) { } 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. + *

+ * Two threads racing on the very first error may both load the template. That is harmless - + * FreeMarker caches templates in its own configuration - and cheaper than locking a path taken + * once per application. + * + * @return the problem report template + * @throws IOException if the template cannot be loaded + */ + protected Template getTemplate() throws IOException { + Template result = template; + if (result == null) { + result = freemarkerManager.getConfiguration(servletContext).getTemplate(ERROR_TEMPLATE); + template = result; } + return result; } public void handleError(HttpServletRequest request, HttpServletResponse response, int code, Exception e) { @@ -115,7 +133,7 @@ protected void handleErrorInDevMode(HttpServletResponse response, int code, Exce } 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 6d44c7dac0..f1f5210477 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 void testHandleErrorIllegalStateExceptionDevMode() { 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); From 6666929ed306e1bf51b0a424964abefc5517e057 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 24 Aug 2026 12:55:35 +0200 Subject: [PATCH 2/2] 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 --- .../DefaultDispatcherErrorHandler.java | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 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 290db77539..779ca7cd1f 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandler.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandler.java @@ -52,7 +52,7 @@ public class DefaultDispatcherErrorHandler implements DispatcherErrorHandler { private FreemarkerManager freemarkerManager; private boolean devMode; private ServletContext servletContext; - private volatile Template template; + private Template template; @Inject public void setFreemarkerManager(FreemarkerManager freemarkerManager) { @@ -73,20 +73,18 @@ public void init(ServletContext ctx) { * rendered in devMode, so a production application should never build a FreeMarker * configuration on its behalf. *

- * Two threads racing on the very first error may both load the template. That is harmless - - * FreeMarker caches templates in its own configuration - and cheaper than locking a path taken - * once per application. + * 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 Template getTemplate() throws IOException { - Template result = template; - if (result == null) { - result = freemarkerManager.getConfiguration(servletContext).getTemplate(ERROR_TEMPLATE); - template = result; + protected synchronized Template getTemplate() throws IOException { + if (template == null) { + template = freemarkerManager.getConfiguration(servletContext).getTemplate(ERROR_TEMPLATE); } - return result; + return template; } public void handleError(HttpServletRequest request, HttpServletResponse response, int code, Exception e) {