From c2eadc2bcefccc882f2445f1201043185e4a6624 Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com> Date: Mon, 21 Dec 2020 19:38:53 -0500 Subject: [PATCH 1/2] Initial attempt to address WW-5101 - Add comments to methods related to ResrouceBundle cache clearing. - Correct clearTomcatCache() so that it acts as intended for Tomcat 7-10. - Add clearResourceBundleClassloaderCaches() method that uses standard Java API calls to perform the cache clearing. - Adjust clearMap() logic to only change accessiblity when required. --- .../util/AbstractLocalizedTextProvider.java | 87 +++++++++++++++---- 1 file changed, 69 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/util/AbstractLocalizedTextProvider.java b/core/src/main/java/com/opensymphony/xwork2/util/AbstractLocalizedTextProvider.java index a8ccab5e4..f214a9289 100644 --- a/core/src/main/java/com/opensymphony/xwork2/util/AbstractLocalizedTextProvider.java +++ b/core/src/main/java/com/opensymphony/xwork2/util/AbstractLocalizedTextProvider.java @@ -53,6 +53,9 @@ abstract class AbstractLocalizedTextProvider implements LocalizedTextProvider { public static final String STRUTS_MESSAGES_BUNDLE = "org/apache/struts2/struts-messages"; private static final String TOMCAT_RESOURCE_ENTRIES_FIELD = "resourceEntries"; + private static final String TOMCAT_PARALLEL_WEBAPP_CLASSLOADER = "org.apache.catalina.loader.ParallelWebappClassLoader"; + private static final String TOMCAT_WEBAPP_CLASSLOADER = "org.apache.catalina.loader.WebappClassLoader"; + private static final String TOMCAT_WEBAPP_CLASSLOADER_BASE = "org.apache.catalina.loader.WebappClassLoaderBase"; private static final String RELOADED = "com.opensymphony.xwork2.util.LocalizedTextProvider.reloaded"; protected final ConcurrentMap bundlesMap = new ConcurrentHashMap<>(); @@ -286,13 +289,7 @@ abstract class AbstractLocalizedTextProvider implements LocalizedTextProvider { } if (!reloaded) { bundlesMap.clear(); - try { - clearMap(ResourceBundle.class, null, "cacheList"); - } catch (NoSuchFieldException e) { - // happens in IBM JVM, that has a different ResourceBundle impl - // it has a 'cache' member - clearMap(ResourceBundle.class, null, "cache"); - } + clearResourceBundleClassloaderCaches(); // now, for the true and utter hack, if we're running in tomcat, clear // it's class loader resource cache as well. @@ -308,36 +305,90 @@ abstract class AbstractLocalizedTextProvider implements LocalizedTextProvider { } } + /** + * A helper method for {@link ResourceBundle} bundle reload logic. + * + * Uses standard {@link ResourceBundle} methods to clear the bundle caches for the + * {@link ClassLoader} instances that this class is aware of at the time of the call. + * + * The clearCache() methods have been available since Java 1.6, so + * it is anticipated the logic will work on any subsequent JVM versions. + * + * @since 2.6 + */ + private void clearResourceBundleClassloaderCaches() { + final ClassLoader ccl = getCurrentThreadContextClassLoader(); + ResourceBundle.clearCache(); // Bundles loaded by the caller's classloader. + ResourceBundle.clearCache(ccl); // Bundles loaded by the context classloader (may be the same). + // Clear the bundle cache for any non-null delegated classloaders. + delegatedClassLoaderMap.forEach( (key, value) -> { if (value != null) ResourceBundle.clearCache(value) ;} ); + } + + /** + * "Hacky" helper method that attempts to clear the Tomcat ResourceEntry + * {@link Map} using knowledge of the Tomcat source code. + * + * It relies on the {@link #TOMCAT_RESOURCE_ENTRIES_FIELD} field name, base class name + * {@link #TOMCAT_WEBAPP_CLASSLOADER_BASE}. and descendant class names {@link #TOMCAT_WEBAPP_CLASSLOADER}, + * {@link #TOMCAT_PARALLEL_WEBAPP_CLASSLOADER}, to keep the values identified in the constants. + * It appears to be valid for Tomcat versions 7-10 so far, but could become invalid at any time in the future + * when the resource handling logic in Tomcat changes. + * + * Note: With Java 9+, calling this method may result in "Illegal reflective access" warnings. Be aware + * its logic may fail in a future version of Java that blocks the reflection calls needed for this method. + * { + */ private void clearTomcatCache() { ClassLoader loader = getCurrentThreadContextClassLoader(); // no need for compilation here. Class cl = loader.getClass(); + Class superCl = cl.getSuperclass(); try { - if ("org.apache.catalina.loader.WebappClassLoader".equals(cl.getName())) { - clearMap(cl, loader, TOMCAT_RESOURCE_ENTRIES_FIELD); + if ( (TOMCAT_WEBAPP_CLASSLOADER.equals(cl.getName()) || TOMCAT_PARALLEL_WEBAPP_CLASSLOADER.equals(cl.getName())) && + (superCl != null && TOMCAT_WEBAPP_CLASSLOADER_BASE.equals(superCl.getName())) ) { + // The classloader name and superclass name match the expecations for a Tomcat classloader. + // Expect the classloader superclass to have the field, otherwise fallback to the classloader class if the field is not found. + clearMap(superCl, loader, TOMCAT_RESOURCE_ENTRIES_FIELD); + LOG.debug("Cleared tomcat cache via classloader's parent class."); } else { LOG.debug("Class loader {} is not tomcat loader.", cl.getName()); } } catch (NoSuchFieldException nsfe) { - if ("org.apache.catalina.loader.WebappClassLoaderBase".equals(cl.getSuperclass().getName())) { - LOG.debug("Base class {} doesn't contain '{}' field, trying with parent!", cl.getName(), TOMCAT_RESOURCE_ENTRIES_FIELD, nsfe); - try { - clearMap(cl.getSuperclass(), loader, TOMCAT_RESOURCE_ENTRIES_FIELD); - } catch (Exception e) { - LOG.warn("Couldn't clear tomcat cache using {}", cl.getSuperclass().getName(), e); - } + LOG.debug("Parent class {} doesn't contain '{}' field, trying with base!", superCl.getName(), TOMCAT_RESOURCE_ENTRIES_FIELD, nsfe); + try { + clearMap(cl, loader, TOMCAT_RESOURCE_ENTRIES_FIELD); + LOG.debug("Cleared tomcat cache via classloader's class."); + } catch (Exception e) { + LOG.warn("Couldn't clear tomcat cache using {}", cl.getName(), e); } } catch (Exception e) { - LOG.warn("Couldn't clear tomcat cache", cl.getName(), e); + LOG.warn("Couldn't clear tomcat cache using {}", superCl.getName(), e); } } + /** + * Helper method that is intended to clear a {@link Map} instance by name. + * + * This method relies on reflection to perform its operations, and may be blocked in Java 9 and later, + * depending on the accessibility of the field. + * + * @param cl The {@link Class} of the obj parameter. + * @param obj The {@link Object} from which the named field is to be extracted (may be null for a static field). + * @param name The name of the field containing a {@link Map} reference. + * @throws NoSuchFieldException if a field accessed by this call does not exist. + * @throws IllegalAccessException if a field, method or or class accessed by this call cannot be accessed. + * @throws NoSuchMethodException if a method accessed by this call does not exist. + * @throws InvocationTargetException if a method accessed by this call fails invocation. + */ private void clearMap(Class cl, Object obj, String name) throws NoSuchFieldException, IllegalAccessException, NoSuchMethodException, InvocationTargetException { Field field = cl.getDeclaredField(name); - field.setAccessible(true); + + if (!field.isAccessible()) { + field.setAccessible(true); // Change state only if necessary. + } Object cache = field.get(obj); From 2020e2fa2548cc40be25b08625afb7383756ddd7 Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com> Date: Sun, 27 Dec 2020 16:21:47 -0500 Subject: [PATCH 2/2] Updated commit based on feedback, plus a few minor changes: - Changed formatting as per review. - Added null guard for unlikely dereference in clearTomcatCache() logging statement. - Added a unit test for basic correctness check and better code coverage. --- .../util/AbstractLocalizedTextProvider.java | 7 +- .../util/StrutsLocalizedTextProviderTest.java | 78 +++++++++++++++++++ 2 files changed, 81 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/util/AbstractLocalizedTextProvider.java b/core/src/main/java/com/opensymphony/xwork2/util/AbstractLocalizedTextProvider.java index f214a9289..6ed6202d8 100644 --- a/core/src/main/java/com/opensymphony/xwork2/util/AbstractLocalizedTextProvider.java +++ b/core/src/main/java/com/opensymphony/xwork2/util/AbstractLocalizedTextProvider.java @@ -336,7 +336,6 @@ abstract class AbstractLocalizedTextProvider implements LocalizedTextProvider { * * Note: With Java 9+, calling this method may result in "Illegal reflective access" warnings. Be aware * its logic may fail in a future version of Java that blocks the reflection calls needed for this method. - * { */ private void clearTomcatCache() { ClassLoader loader = getCurrentThreadContextClassLoader(); @@ -345,8 +344,8 @@ abstract class AbstractLocalizedTextProvider implements LocalizedTextProvider { Class superCl = cl.getSuperclass(); try { - if ( (TOMCAT_WEBAPP_CLASSLOADER.equals(cl.getName()) || TOMCAT_PARALLEL_WEBAPP_CLASSLOADER.equals(cl.getName())) && - (superCl != null && TOMCAT_WEBAPP_CLASSLOADER_BASE.equals(superCl.getName())) ) { + if ((TOMCAT_WEBAPP_CLASSLOADER.equals(cl.getName()) || TOMCAT_PARALLEL_WEBAPP_CLASSLOADER.equals(cl.getName())) && + (superCl != null && TOMCAT_WEBAPP_CLASSLOADER_BASE.equals(superCl.getName()))) { // The classloader name and superclass name match the expecations for a Tomcat classloader. // Expect the classloader superclass to have the field, otherwise fallback to the classloader class if the field is not found. clearMap(superCl, loader, TOMCAT_RESOURCE_ENTRIES_FIELD); @@ -363,7 +362,7 @@ abstract class AbstractLocalizedTextProvider implements LocalizedTextProvider { LOG.warn("Couldn't clear tomcat cache using {}", cl.getName(), e); } } catch (Exception e) { - LOG.warn("Couldn't clear tomcat cache using {}", superCl.getName(), e); + LOG.warn("Couldn't clear tomcat cache using {}", (superCl != null ? superCl.getName() : null), e); } } diff --git a/core/src/test/java/com/opensymphony/xwork2/util/StrutsLocalizedTextProviderTest.java b/core/src/test/java/com/opensymphony/xwork2/util/StrutsLocalizedTextProviderTest.java index aef469dd8..2a3038ff0 100644 --- a/core/src/test/java/com/opensymphony/xwork2/util/StrutsLocalizedTextProviderTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/util/StrutsLocalizedTextProviderTest.java @@ -327,6 +327,50 @@ public class StrutsLocalizedTextProviderTest extends XWorkTestCase { testStrutsLocalizedTextProvider.callClearMissingBundlesCache(); } + /** + * Unit test to confirm the basic behaviour of bundle reload methods provided to + * StrutsLocalizedTextProvider (from AbstractLocalizedTextProvider). + * + * @since 2.6 + */ + public void testLocalizedTextProviderReloadMethods() { + TestStrutsLocalizedTextProvider testStrutsLocalizedTextProvider = new TestStrutsLocalizedTextProvider(); + assertTrue("testStrutsLocalizedTextProvider not instance of AbstractLocalizedTextProvider ?", + testStrutsLocalizedTextProvider instanceof AbstractLocalizedTextProvider); + assertEquals("testStrutsLocalizedTextProvider starting default bundle map size not 0 before any retrievals ?", + 0, testStrutsLocalizedTextProvider.currentBundlesMapSize()); + + // Access the two default bundles to populate their cache entries and test bundle map size. + ResourceBundle tempBundle = testStrutsLocalizedTextProvider.findResourceBundle( + TestStrutsLocalizedTextProvider.XWORK_MESSAGES_BUNDLE, Locale.ENGLISH); + assertNotNull("XWORK_MESSAGES_BUNDLE retrieval null ?", tempBundle); + tempBundle = testStrutsLocalizedTextProvider.findResourceBundle( + TestStrutsLocalizedTextProvider.STRUTS_MESSAGES_BUNDLE, Locale.ENGLISH); + assertNotNull("STRUTS_MESSAGES_BUNDLE retrieval null ?", tempBundle); + assertEquals("testStrutsLocalizedTextProvider bundle map size not 2 after retrievals ?", + 2, testStrutsLocalizedTextProvider.currentBundlesMapSize()); + + // Force a bundle reload call for code coverage and to confirm it causes the bundle map to be emptied. + assertNotNull("ActionContext is somehow null ?", ActionContext.getContext()); + boolean bundlesReloadedBeforeCall = testStrutsLocalizedTextProvider.getBundlesReloadedIndicatorValue(); + assertFalse("Bundles reload value true before forced reload ?", bundlesReloadedBeforeCall); + testStrutsLocalizedTextProvider.callReloadBundlesForceReload(); + boolean bundlesReloadedAfterCall = testStrutsLocalizedTextProvider.getBundlesReloadedIndicatorValue(); + assertTrue("Bundles reload value false after forced reload ?", bundlesReloadedAfterCall); + assertEquals("testStrutsLocalizedTextProvider bundle map size not 0 after reload (which should clear it) ?", + 0, testStrutsLocalizedTextProvider.currentBundlesMapSize()); + + // Access the two default bundles again (after reload) to populate their cache entries and test bundle map size. + tempBundle = testStrutsLocalizedTextProvider.findResourceBundle( + TestStrutsLocalizedTextProvider.XWORK_MESSAGES_BUNDLE, Locale.ENGLISH); + assertNotNull("XWORK_MESSAGES_BUNDLE retrieval null ?", tempBundle); + tempBundle = testStrutsLocalizedTextProvider.findResourceBundle( + TestStrutsLocalizedTextProvider.STRUTS_MESSAGES_BUNDLE, Locale.ENGLISH); + assertNotNull("STRUTS_MESSAGES_BUNDLE retrieval null ?", tempBundle); + assertEquals("testStrutsLocalizedTextProvider bundle map size not 2 after retrievals ?", + 2, testStrutsLocalizedTextProvider.currentBundlesMapSize()); + } + @Override protected void setUp() throws Exception { super.setUp(); @@ -352,6 +396,13 @@ public class StrutsLocalizedTextProviderTest extends XWorkTestCase { */ class TestStrutsLocalizedTextProvider extends StrutsLocalizedTextProvider { + /** + * Some test correctness depends on this {@link #RELOADED} value matching that of the private ancestor + * field {@link AbstractLocalizedTextProvider#RELOADED}. If the ancestor field value changes, ensure this + * field's value is updated to match it exactly. + */ + private static final String RELOADED = "com.opensymphony.xwork2.util.LocalizedTextProvider.reloaded"; + public void callClearBundleNoLocale(String bundleName) { super.clearBundle(bundleName); } @@ -367,5 +418,32 @@ public class StrutsLocalizedTextProviderTest extends XWorkTestCase { public int currentBundlesMapSize() { return super.bundlesMap.size(); } + + /** + * Attempt to force the resource bundles to be reloaded, even if configuration would otherwise prevent it. + * It will preserve the current reloadBundles state, attempt to force a reload and then restore the + * original reloadBundles value. + */ + public void callReloadBundlesForceReload() { + final boolean originalReloadState = super.reloadBundles; + try { + super.setReloadBundles(Boolean.TRUE.toString()); + super.reloadBundles(); + } finally { + super.setReloadBundles(Boolean.toString(originalReloadState)); + } + } + + /** + * Returns the value of the resource bundles reloaded state from the context, provided that one was + * previously set. If no value is found, the result will be false (same as if bundles had not been reloaded). + * + * @return true if resource bundles reloaded indicator is true, false otherwise (including if value was never set). + */ + public boolean getBundlesReloadedIndicatorValue() { + final ActionContext actionContext = ActionContext.getContext(); + final Object reloadedObject = actionContext.get(RELOADED); + return ((reloadedObject instanceof Boolean) ? ((Boolean) reloadedObject).booleanValue() : false); + } } }