WW-5382 Fix stale injections in Dispatcher

core/src/main/java/com/opensymphony/xwork2/XWorkJUnit4TestCase.java

@@ -51,10 +51,6 @@ public void setUp() throws Exception {
     @After
     public void tearDown() throws Exception {
         XWorkTestCaseHelper.tearDown(configurationManager);
-        configurationManager = null;
-        configuration = null;
-        container = null;
-        actionProxyFactory = null;
     }
 
     protected void loadConfigurationProviders(ConfigurationProvider... providers) {

core/src/main/java/com/opensymphony/xwork2/XWorkTestCase.java

@@ -64,10 +64,6 @@ protected void setUp() throws Exception {
     @Override
     protected void tearDown() throws Exception {
         XWorkTestCaseHelper.tearDown(configurationManager);
-        configurationManager = null;
-        configuration = null;
-        container = null;
-        actionProxyFactory = null;
     }
 
     protected void loadConfigurationProviders(ConfigurationProvider... providers) {

core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java

@@ -325,12 +325,8 @@ public Class<? extends Configuration> type() {
     }
 
     protected ActionContext setContext(Container cont) {
-        ActionContext context = ActionContext.getContext();
-        if (context == null) {
-            ValueStack vs = cont.getInstance(ValueStackFactory.class).createValueStack();
-            context = ActionContext.of(vs.getContext()).bind();
-        }
-        return context;
+        ValueStack vs = cont.getInstance(ValueStackFactory.class).createValueStack();
+        return ActionContext.of(vs.getContext()).bind();
     }
 
     protected Container createBootstrapContainer(List<ContainerProvider> providers) {

core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java

@@ -23,25 +23,25 @@
 /**
  * Simple class to hold Container instance per thread to minimise number of attempts
  * to read configuration and build each time a new configuration.
- *
+ * <p>
  * As ContainerHolder operates just per thread (which means per request) there is no need
  * to check if configuration changed during the same request. If changed between requests,
  * first call to store Container in ContainerHolder will be with the new configuration.
  */
 class ContainerHolder {
 
-    private static ThreadLocal<Container> instance = new ThreadLocal<>();
+    private static final ThreadLocal<Container> instance = new ThreadLocal<>();
 
-    public static void store(Container instance) {
-        ContainerHolder.instance.set(instance);
+    public static void store(Container newInstance) {
+        instance.set(newInstance);
     }
 
     public static Container get() {
-        return ContainerHolder.instance.get();
+        return instance.get();
     }
 
     public static void clear() {
-        ContainerHolder.instance.remove();
+        instance.remove();
     }
 
 }

core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java

@@ -64,6 +64,7 @@
 import org.apache.struts2.config.StrutsJavaConfiguration;
 import org.apache.struts2.config.StrutsJavaConfigurationProvider;
 import org.apache.struts2.config.StrutsXmlConfigurationProvider;
+import org.apache.struts2.dispatcher.mapper.ActionMapper;
 import org.apache.struts2.dispatcher.mapper.ActionMapping;
 import org.apache.struts2.dispatcher.multipart.MultiPartRequest;
 import org.apache.struts2.dispatcher.multipart.MultiPartRequestWrapper;
@@ -119,6 +120,12 @@ public class Dispatcher {
      */
     private static final List<DispatcherListener> dispatcherListeners = new CopyOnWriteArrayList<>();
 
+    /**
+     * This field exists so {@link #getContainer()} can determine whether to (re-)inject this instance in the case of
+     * a {@link ConfigurationManager} reload.
+     */
+    private Container injectedContainer;
+
     /**
      * Store state of StrutsConstants.STRUTS_DEVMODE setting.
      */
@@ -144,11 +151,6 @@ public class Dispatcher {
      */
     private String multipartSaveDir;
 
-    /**
-     * Stores the value of {@link StrutsConstants#STRUTS_MULTIPART_PARSER} setting
-     */
-    private String multipartHandlerName;
-
     /**
      * Stores the value of {@link StrutsConstants#STRUTS_MULTIPART_ENABLED}
      */
@@ -192,6 +194,11 @@ public class Dispatcher {
      * Store ConfigurationManager instance, set on init.
      */
     protected ConfigurationManager configurationManager;
+    private ObjectFactory objectFactory;
+    private ActionProxyFactory actionProxyFactory;
+    private LocaleProviderFactory localeProviderFactory;
+    private StaticContentLoader staticContentLoader;
+    private ActionMapper actionMapper;
 
     /**
      * Provide the dispatcher instance for the current thread.
@@ -211,6 +218,13 @@ public static void setInstance(Dispatcher instance) {
         Dispatcher.instance.set(instance);
     }
 
+    /**
+     * Removes the dispatcher instance for this thread.
+     */
+    public static void clearInstance() {
+        Dispatcher.instance.remove();
+    }
+
     /**
      * Add a dispatcher lifecycle listener.
      *
@@ -306,9 +320,12 @@ public void setMultipartSaveDir(String val) {
         multipartSaveDir = val;
     }
 
-    @Inject(StrutsConstants.STRUTS_MULTIPART_PARSER)
+    /**
+     * @deprecated since 6.4.0, no replacement.
+     */
+    @Deprecated
     public void setMultipartHandler(String val) {
-        multipartHandlerName = val;
+        // no-op
     }
 
     @Inject(value = StrutsConstants.STRUTS_MULTIPART_ENABLED, required = false)
@@ -326,11 +343,21 @@ public void setValueStackFactory(ValueStackFactory valueStackFactory) {
         this.valueStackFactory = valueStackFactory;
     }
 
+    public ValueStackFactory getValueStackFactory() {
+        return valueStackFactory;
+    }
+
     @Inject(StrutsConstants.STRUTS_HANDLE_EXCEPTION)
     public void setHandleException(String handleException) {
         this.handleException = Boolean.parseBoolean(handleException);
     }
 
+    @Inject(StrutsConstants.STRUTS_DISPATCHER_PARAMETERSWORKAROUND)
+    public void setDispatchersParametersWorkaround(String dispatchersParametersWorkaround) {
+        this.paramsWorkaroundEnabled = Boolean.parseBoolean(dispatchersParametersWorkaround)
+                || (servletContext != null && StringUtils.contains(servletContext.getServerInfo(), "WebLogic"));
+    }
+
     public boolean isHandleException() {
         return handleException;
     }
@@ -340,12 +367,48 @@ public void setDispatcherErrorHandler(DispatcherErrorHandler errorHandler) {
         this.errorHandler = errorHandler;
     }
 
+    @Inject
+    public void setObjectFactory(ObjectFactory objectFactory) {
+        this.objectFactory = objectFactory;
+    }
+
+    @Inject
+    public void setActionProxyFactory(ActionProxyFactory actionProxyFactory) {
+        this.actionProxyFactory = actionProxyFactory;
+    }
+
+    public ActionProxyFactory getActionProxyFactory() {
+        return actionProxyFactory;
+    }
+
+    @Inject
+    public void setLocaleProviderFactory(LocaleProviderFactory localeProviderFactory) {
+        this.localeProviderFactory = localeProviderFactory;
+    }
+
+    @Inject
+    public void setStaticContentLoader(StaticContentLoader staticContentLoader) {
+        this.staticContentLoader = staticContentLoader;
+    }
+
+    public StaticContentLoader getStaticContentLoader() {
+        return staticContentLoader;
+    }
+
+    @Inject
+    public void setActionMapper(ActionMapper actionMapper) {
+        this.actionMapper = actionMapper;
+    }
+
+    public ActionMapper getActionMapper() {
+        return actionMapper;
+    }
+
     /**
      * Releases all instances bound to this dispatcher instance.
      */
     public void cleanup() {
         // clean up ObjectFactory
-        ObjectFactory objectFactory = getContainer().getInstance(ObjectFactory.class);
         if (objectFactory == null) {
             LOG.warn("Object Factory is null, something is seriously wrong, no clean up will be performed");
         }
@@ -359,7 +422,7 @@ public void cleanup() {
         }
 
         // clean up Dispatcher itself for this thread
-        instance.set(null);
+        instance.remove();
         servletContext.setAttribute(StrutsStatics.SERVLET_DISPATCHER, null);
 
         // clean up DispatcherListeners
@@ -532,21 +595,6 @@ private void init_DeferredXmlConfigurations() {
         loadConfigPaths("struts-deferred.xml");
     }
 
-    private Container init_PreloadConfiguration() {
-        return getContainer();
-    }
-
-    private void init_CheckWebLogicWorkaround(Container container) {
-        // test whether param-access workaround needs to be enabled
-        if (servletContext != null && StringUtils.contains(servletContext.getServerInfo(), "WebLogic")) {
-            LOG.info("WebLogic server detected. Enabling Struts parameter access work-around.");
-            paramsWorkaroundEnabled = true;
-        } else {
-            paramsWorkaroundEnabled = "true".equals(container.getInstance(String.class,
-                StrutsConstants.STRUTS_DISPATCHER_PARAMETERSWORKAROUND));
-        }
-    }
-
     /**
      * Load configurations, including both XML and zero-configuration strategies,
      * and update optional settings, including whether to reload configurations and resource files.
@@ -567,9 +615,7 @@ public void init() {
             init_AliasStandardObjects(); // [7]
             init_DeferredXmlConfigurations();
 
-            Container container = init_PreloadConfiguration();
-            container.inject(this);
-            init_CheckWebLogicWorkaround(container);
+            getContainer(); // Inject this instance
 
             if (!dispatcherListeners.isEmpty()) {
                 for (DispatcherListener l : dispatcherListeners) {
@@ -689,7 +735,6 @@ protected ActionProxy prepareActionProxy(Map<String, Object> extraContext, Strin
     }
 
     protected ActionProxy createActionProxy(String namespace, String name, String method, Map<String, Object> extraContext) {
-        ActionProxyFactory actionProxyFactory = getContainer().getInstance(ActionProxyFactory.class);
         return actionProxyFactory.createActionProxy(namespace, name, method, extraContext, true, false);
     }
 
@@ -865,6 +910,7 @@ protected String getSaveDir() {
      * @param response The response
      */
     public void prepare(HttpServletRequest request, HttpServletResponse response) {
+        getContainer(); // Init ContainerHolder and reinject this instance IF ConfigurationManager was reloaded
         String encoding = null;
         if (defaultEncoding != null) {
             encoding = defaultEncoding;
@@ -937,15 +983,12 @@ public HttpServletRequest wrapRequest(HttpServletRequest request) throws IOExcep
         }
 
         if (isMultipartSupportEnabled(request) && isMultipartRequest(request)) {
-            MultiPartRequest multiPartRequest = getMultiPartRequest();
-            LocaleProviderFactory localeProviderFactory = getContainer().getInstance(LocaleProviderFactory.class);
-
             request = new MultiPartRequestWrapper(
-                multiPartRequest,
-                request,
-                getSaveDir(),
-                localeProviderFactory.createLocaleProvider(),
-                disableRequestAttributeValueStackLookup
+                    getMultiPartRequest(),
+                    request,
+                    getSaveDir(),
+                    localeProviderFactory.createLocaleProvider(),
+                    disableRequestAttributeValueStackLookup
             );
         } else {
             request = new StrutsRequestWrapper(request, disableRequestAttributeValueStackLookup);
@@ -988,18 +1031,7 @@ protected boolean isMultipartRequest(HttpServletRequest request) {
      * @return a multi part request object
      */
     protected MultiPartRequest getMultiPartRequest() {
-        MultiPartRequest mpr = null;
-        //check for alternate implementations of MultiPartRequest
-        Set<String> multiNames = getContainer().getInstanceNames(MultiPartRequest.class);
-        for (String multiName : multiNames) {
-            if (multiName.equals(multipartHandlerName)) {
-                mpr = getContainer().getInstance(MultiPartRequest.class, multiName);
-            }
-        }
-        if (mpr == null) {
-            mpr = getContainer().getInstance(MultiPartRequest.class);
-        }
-        return mpr;
+        return getContainer().getInstance(MultiPartRequest.class);
     }
 
     /**
@@ -1063,27 +1095,26 @@ public ConfigurationManager getConfigurationManager() {
     }
 
     /**
-     * Expose the dependency injection container.
+     * Exposes a thread-cached reference of the dependency injection container. If the container is found to have
+     * changed since the last time it was cached, this Dispatcher instance is re-injected to ensure no stale
+     * configuration/dependencies persist.
+     * <p>
+     * A non-cached reference can be obtained by calling {@link #getConfigurationManager()}.
      *
      * @return Our dependency injection container
      */
     public Container getContainer() {
-        if (ContainerHolder.get() != null) {
-            return ContainerHolder.get();
-        }
-        ConfigurationManager mgr = getConfigurationManager();
-        if (mgr == null) {
-            throw new IllegalStateException("The configuration manager shouldn't be null");
-        } else {
-            Configuration config = mgr.getConfiguration();
-            if (config == null) {
-                throw new IllegalStateException("Unable to load configuration");
-            } else {
-                Container container = config.getContainer();
-                ContainerHolder.store(container);
-                return container;
+        if (ContainerHolder.get() == null) {
+            try {
+                ContainerHolder.store(getConfigurationManager().getConfiguration().getContainer());
+            } catch (NullPointerException e) {
+                throw new IllegalStateException("ConfigurationManager and/or Configuration should not be null", e);
             }
         }
+        if (injectedContainer != ContainerHolder.get()) {
+            injectedContainer = ContainerHolder.get();
+            injectedContainer.inject(this);
+        }
+        return ContainerHolder.get();
     }
-
 }

core/src/main/java/org/apache/struts2/dispatcher/ExecuteOperations.java

@@ -18,8 +18,8 @@
  */
 package org.apache.struts2.dispatcher;
 
-import org.apache.struts2.dispatcher.mapper.ActionMapping;
 import org.apache.struts2.RequestUtils;
+import org.apache.struts2.dispatcher.mapper.ActionMapping;
 
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
@@ -54,7 +54,7 @@ public boolean executeStaticResourceRequest(HttpServletRequest request, HttpServ
             resourcePath = request.getPathInfo();
         }
 
-        StaticContentLoader staticResourceLoader = dispatcher.getContainer().getInstance(StaticContentLoader.class);
+        StaticContentLoader staticResourceLoader = dispatcher.getStaticContentLoader();
         if (staticResourceLoader.canHandle(resourcePath)) {
             staticResourceLoader.findStaticResource(resourcePath, request, response);
             // The framework did its job here

core/src/main/java/org/apache/struts2/dispatcher/InitOperations.java

@@ -57,7 +57,7 @@ public Dispatcher initDispatcher(HostConfig filterConfig) {
      * @return the static content loader
      */
     public StaticContentLoader initStaticContentLoader(HostConfig filterConfig, Dispatcher dispatcher) {
-        StaticContentLoader loader = dispatcher.getContainer().getInstance(StaticContentLoader.class);
+        StaticContentLoader loader = dispatcher.getStaticContentLoader();
         loader.setHostConfig(filterConfig);
         return loader;
     }