WW-5690 perf(dispatcher): load the dev-mode error template on first use - #1864
Merged
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes WW-5690
DefaultDispatcherErrorHandler.init()built a FreeMarker configuration and loaded/org/apache/struts2/dispatcher/error.ftlon every startup — including production, where the problem report is never rendered.handleError()only reaches it when devMode is on; otherwise it delegates to the container's error page.So every Struts application today pays to initialise FreeMarker for a page most of them never show.
Change
init()now just stores theServletContext. A newprotected synchronized getTemplate()loads the template on first use and caches it.The lock is deliberate rather than lock-free: this path only runs when devMode is on and a request has already failed, so it is never hot, and
FreemarkerManager.getConfigurationis itselfsynchronized— the path was already taking a lock. A plain field behind a synchronized getter is simpler to verify than avolatileone, and avoidsjava:S3077(volatile publishes the reference, not the object's state, andTemplateextendsConfigurable).One behavioural change
A missing or unparsable
error.ftlused to throwStrutsExceptionfrominit()and fail the application at boot. It now surfaces on the first dev-mode error, where the existingcatch (Exception exp)inhandleErrorInDevMode()already degrades tosendError(code, "Unable to show problem report: ...").I'd argue that's an improvement — a dev-only template shouldn't stop a production application starting — but it is a change in failure timing and worth a reviewer's eye.
Scope
Startup cost only. The
FreemarkerManagerinjection, thefreemarker.template.Templateimport anderror.ftlitself all stay — removing them is WW-5693, targeted at 8.0.0 because it deletes a template that can be overridden on the classpath.Both are part of the lean-core work tracked in WW-5689, alongside WW-5691 and WW-5692 for the other two places core reaches into
views.freemarker.Tests
Two added to
DefaultDispatcherErrorHandlerTest, using aFreemarkerManagersubclass that records whethergetConfigurationwas called (no mocking framework needed — it delegates tosuper, so the template really renders):testInitDoesNotLoadErrorTemplate— assertsinit()leaves FreeMarker untouched. Verified failing before the change, on the assertion, which is what makes it a real regression guard.testErrorTemplateLoadedOnFirstDevModeError— asserts the deferred load actually happens, so the report can't silently stop rendering.The four existing tests are unchanged and still pass. Full
coresuite: 3197 tests, 0 failures, 0 errors.🤖 Generated with Claude Code