You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by GitBox <gi...@apache.org> on 2020/12/22 00:44:36 UTC
[GitHub] [struts] JCgH4164838Gh792C124B5 opened a new pull request #460: Initial attempt to address WW-5101
JCgH4164838Gh792C124B5 opened a new pull request #460:
URL: https://github.com/apache/struts/pull/460
Initial attempt to address WW-5101
- Add comments to methods related to ResourceBundle 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] coveralls edited a comment on pull request #460: Initial attempt to address WW-5101
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #460:
URL: https://github.com/apache/struts/pull/460#issuecomment-749278370
[](https://coveralls.io/builds/35958762)
Coverage increased (+0.03%) to 49.811% when pulling **2020e2fa2548cc40be25b08625afb7383756ddd7 on JCgH4164838Gh792C124B5:localS2_26_WW-5101_cleanupfix1** into **7532d2fb0d6081a12c2a48ec854a81a8b718be62 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] sepe81 commented on a change in pull request #460: Initial attempt to address WW-5101
Posted by GitBox <gi...@apache.org>.
sepe81 commented on a change in pull request #460:
URL: https://github.com/apache/struts/pull/460#discussion_r547988638
##########
File path: core/src/main/java/com/opensymphony/xwork2/util/AbstractLocalizedTextProvider.java
##########
@@ -308,36 +305,90 @@ protected void reloadBundles(Map<String, Object> context) {
}
}
+ /**
+ * 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 <code>clearCache()</code> 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 <code>ResourceEntry</code>
+ * {@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.
+ * {<code></code>
+ */
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())) &&
Review comment:
```suggestion
if ((TOMCAT_WEBAPP_CLASSLOADER.equals(cl.getName()) || TOMCAT_PARALLEL_WEBAPP_CLASSLOADER.equals(cl.getName())) &&
```
consistent formatting
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] sepe81 commented on a change in pull request #460: Initial attempt to address WW-5101
Posted by GitBox <gi...@apache.org>.
sepe81 commented on a change in pull request #460:
URL: https://github.com/apache/struts/pull/460#discussion_r547989077
##########
File path: core/src/main/java/com/opensymphony/xwork2/util/AbstractLocalizedTextProvider.java
##########
@@ -308,36 +305,90 @@ protected void reloadBundles(Map<String, Object> context) {
}
}
+ /**
+ * 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 <code>clearCache()</code> 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 <code>ResourceEntry</code>
+ * {@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.
+ * {<code></code>
+ */
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())) ) {
Review comment:
```suggestion
(superCl != null && TOMCAT_WEBAPP_CLASSLOADER_BASE.equals(superCl.getName()))) {
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] coveralls commented on pull request #460: Initial attempt to address WW-5101
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #460:
URL: https://github.com/apache/struts/pull/460#issuecomment-749278370
[](https://coveralls.io/builds/35878335)
Coverage decreased (-0.005%) to 49.774% when pulling **c2eadc2bcefccc882f2445f1201043185e4a6624 on JCgH4164838Gh792C124B5:localS2_26_WW-5101_cleanupfix1** into **7532d2fb0d6081a12c2a48ec854a81a8b718be62 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] sepe81 commented on a change in pull request #460: Initial attempt to address WW-5101
Posted by GitBox <gi...@apache.org>.
sepe81 commented on a change in pull request #460:
URL: https://github.com/apache/struts/pull/460#discussion_r547986265
##########
File path: core/src/main/java/com/opensymphony/xwork2/util/AbstractLocalizedTextProvider.java
##########
@@ -308,36 +305,90 @@ protected void reloadBundles(Map<String, Object> context) {
}
}
+ /**
+ * 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 <code>clearCache()</code> 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 <code>ResourceEntry</code>
+ * {@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.
+ * {<code></code>
Review comment:
```suggestion
```
this line can probably be omitted
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart merged pull request #460: Initial attempt to address WW-5101
Posted by GitBox <gi...@apache.org>.
lukaszlenart merged pull request #460:
URL: https://github.com/apache/struts/pull/460
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] lukaszlenart commented on pull request #460: Initial attempt to address WW-5101
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #460:
URL: https://github.com/apache/struts/pull/460#issuecomment-753484972
No complains, so let's merge!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] JCgH4164838Gh792C124B5 commented on pull request #460: Initial attempt to address WW-5101
Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on pull request #460:
URL: https://github.com/apache/struts/pull/460#issuecomment-749276966
Hello Apache Struts Team.
This is an an initial attempt to address the reported issue. Basic local testing indicates it is working, with no illegal reflective access warnings seen with Java 11. People may decide to eventually remove the `clearTomcatCache()` method in the future, but for the moment this PR attempts to correct its behaviour so that it acts as the previous code and comments indicate it should.
Please let me know if the PR looks OK when you have time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org