You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Christopher Schultz <ch...@christopherschultz.net> on 2023/05/03 19:02:22 UTC
Workaround for misbehaving ClassLoader
All,
Please check this thread for full details:
https://lists.apache.org/thread/of5ozg43zyk729cg311dktjcv3swct26
Briefly, a user reported this NPE:
Apr 29, 2023 5:41:32 PM org.apache.catalina.core.StandardContext
listenerStart
SEVERE: Exception sending context initialized event to listener instance
of class [org.springframework.web.context.ContextLoaderListener]
java.lang.NullPointerException
at
org.apache.catalina.loader.WebappClassLoaderBase$CombinedEnumeration.inc(WebappClassLoaderBase.java:2775)
at
org.apache.catalina.loader.WebappClassLoaderBase$CombinedEnumeration.hasMoreElements(WebappClassLoaderBase.java:2760)
at
org.apache.commons.logging.LogFactory.getConfigurationFile(LogFactory.java:1366)
at
org.apache.commons.logging.LogFactory.getFactory(LogFactory.java:453)
at org.apache.commons.logging.LogFactory.getLog(LogFactory.java:655)
Mark introduced the CombinedEnumeration in
https://github.com/apache/tomcat/commit/fdd35009ead6365cbae84008e63356d6ab88ed40
to merge the resources from the parent ClassLoader with the
WebappClassLoader(Base). His code looks sound to me, but it can expose
what I think of as a bug in the ClassLoader being used as the parent.
ClassLoader.getResources is documented to return an empty Enumeration if
there are no matching resources, but in this case, the ClassLoader is
returning null.
WebappClassLoaderBase assumes that the parent ClassLoader is following
the rules and so grabs the resources from both itself and the parent and
combines them using CombinedEnumeration.
Here is a patch that will fix that:
diff --git a/java/org/apache/catalina/loader/WebappClassLoaderBase.java
b/java/org/apache/catalina/loader/WebappClassLoaderBase.java
index aa747a5873..6f98f6e413 100644
--- a/java/org/apache/catalina/loader/WebappClassLoaderBase.java
+++ b/java/org/apache/catalina/loader/WebappClassLoaderBase.java
@@ -1121,7 +1121,9 @@ public abstract class WebappClassLoaderBase
extends URLClassLoader
// Enumerations are combined depends on how delegation is
configured
boolean delegateFirst = delegate || filter(name, false);
- if (delegateFirst) {
+ if(null == parentResources) {
+ return localResources;
+ } else if (delegateFirst) {
return new CombinedEnumeration(parentResources,
localResources);
} else {
return new CombinedEnumeration(localResources,
parentResources);
But my question is whether or not this is something that Tomcat should
be working-around. IMO the parent ClassLoader is buggy and should be
fixed, but it may be difficult or impossible to fix the parent, so it
may be worth it.
We could even log it including the class name of the offending ClassLoader.
WDYT?
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Workaround for misbehaving ClassLoader
Posted by koteswara Rao Gundapaneni <ko...@gmail.com>.
Dear chris,
I will take care about it and update it accordingly
Regards,
Koti
On Wed, May 3, 2023 at 12:02 PM Christopher Schultz <
chris@christopherschultz.net> wrote:
> All,
>
> Please check this thread for full details:
> https://lists.apache.org/thread/of5ozg43zyk729cg311dktjcv3swct26
>
> Briefly, a user reported this NPE:
>
> Apr 29, 2023 5:41:32 PM org.apache.catalina.core.StandardContext
> listenerStart
> SEVERE: Exception sending context initialized event to listener instance
> of class [org.springframework.web.context.ContextLoaderListener]
> java.lang.NullPointerException
> at
>
> org.apache.catalina.loader.WebappClassLoaderBase$CombinedEnumeration.inc(WebappClassLoaderBase.java:2775)
> at
>
> org.apache.catalina.loader.WebappClassLoaderBase$CombinedEnumeration.hasMoreElements(WebappClassLoaderBase.java:2760)
> at
>
> org.apache.commons.logging.LogFactory.getConfigurationFile(LogFactory.java:1366)
> at
> org.apache.commons.logging.LogFactory.getFactory(LogFactory.java:453)
> at org.apache.commons.logging.LogFactory.getLog(LogFactory.java:655)
>
> Mark introduced the CombinedEnumeration in
>
> https://github.com/apache/tomcat/commit/fdd35009ead6365cbae84008e63356d6ab88ed40
> to merge the resources from the parent ClassLoader with the
> WebappClassLoader(Base). His code looks sound to me, but it can expose
> what I think of as a bug in the ClassLoader being used as the parent.
>
> ClassLoader.getResources is documented to return an empty Enumeration if
> there are no matching resources, but in this case, the ClassLoader is
> returning null.
>
> WebappClassLoaderBase assumes that the parent ClassLoader is following
> the rules and so grabs the resources from both itself and the parent and
> combines them using CombinedEnumeration.
>
> Here is a patch that will fix that:
>
> diff --git a/java/org/apache/catalina/loader/WebappClassLoaderBase.java
> b/java/org/apache/catalina/loader/WebappClassLoaderBase.java
> index aa747a5873..6f98f6e413 100644
> --- a/java/org/apache/catalina/loader/WebappClassLoaderBase.java
> +++ b/java/org/apache/catalina/loader/WebappClassLoaderBase.java
> @@ -1121,7 +1121,9 @@ public abstract class WebappClassLoaderBase
> extends URLClassLoader
> // Enumerations are combined depends on how delegation is
> configured
> boolean delegateFirst = delegate || filter(name, false);
>
> - if (delegateFirst) {
> + if(null == parentResources) {
> + return localResources;
> + } else if (delegateFirst) {
> return new CombinedEnumeration(parentResources,
> localResources);
> } else {
> return new CombinedEnumeration(localResources,
> parentResources);
>
> But my question is whether or not this is something that Tomcat should
> be working-around. IMO the parent ClassLoader is buggy and should be
> fixed, but it may be difficult or impossible to fix the parent, so it
> may be worth it.
>
> We could even log it including the class name of the offending ClassLoader.
>
> WDYT?
>
> -chris
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: Workaround for misbehaving ClassLoader
Posted by Mark Thomas <ma...@apache.org>.
On 05/05/2023 21:42, Christopher Schultz wrote:
> Mark,
>
> On 5/4/23 04:09, Mark Thomas wrote:
>> On 03/05/2023 20:02, Christopher Schultz wrote:
>>
>> <snip/>
>>
>>> But my question is whether or not this is something that Tomcat
>>> should be working-around. IMO the parent ClassLoader is buggy and
>>> should be fixed, but it may be difficult or impossible to fix the
>>> parent, so it may be worth it.
>>>
>>> We could even log it including the class name of the offending
>>> ClassLoader.
>>>
>>> WDYT?
>>
>> The general approach we have taken is that we don't work-around bugs
>> in third-party products unless:
>> - the third-party vendor is (known to be) slow to respond to bugs
>> - there is no other viable workaround (including switching vendors)
>> - the bug impacts a reasonable proportion of Tomcat users
>>
>> The complexity of the workaround in Tomcat vs the severity of the
>> issue is also a consideration.
>>
>> We also want to encourage adherence to the relevant specifications.
>>
>> All of the above is subjective.
>>
>> For commercial software, the general idea is to encourage users to put
>> pressure on vendors to fix the bugs rather than expect us to - just
>> because we are more responsive. This is especially true for commercial
>> organizations using commercial software where there should be a
>> support contract in place.
>>
>> For open source software, the general idea is to encourage users to
>> engage with the project concerned. Open a bug, provide a PR,
>> contribute and support that project.
>>
>> The GitHub project in question hasn't had any activity since 2017 and
>> the GitHub organization hasn't had any activity since 2019. There are
>> no forks of the project.
>>
>> It looks like the code has never been released so I am assuming the OP
>> has compiled it locally.
>>
>> The fix in Tomcat is simple, but so is the fix in the problematic
>> library.
>>
>> I think the initial position is that the OP needs to try and get this
>> fixed. Whether that means creating a fork (it is ALv2 so that is easy),
>> seeing if the CodeGerm team can be persuaded to accept a PR, finding
>> an alternative library or something else is up to the OP.
>>
>> Given the options the OP has for addressing this - including a
>> (private) fork, I don't think this is something that should be fixed
>> in Tomcat.
>
> Fair enough.
>
> I do think there is scope for slight optimization in this area as well.
>
> My proposed patch was something like:
>
> if(null == parentResources) {
> return childResouces;
> } else if(parentFirst) {
> return new CombinedEnumeration(parentResources, childResources);
> } else {
> return new CombinedEnumeration(childResources, parentResources);
> }
>
> A simple change of:
>
> if(null == parentResources || !parentResources.hasMoreElements()) {
> return childResouces;
> } ...
>
> means fewer wrapper objects and code executing in the common case (the
> webapp probably provides most of the interesting resources to itself,
> and not from the parent classloader).
>
> In the cases where we construct a new CombinedEnumeration, I would even
> say that we should check to see if either enumeration is empty and try
> not to create a new object unless it's even necessary.
+1 to reducing the wrapping when parent class loader returns an empty
enumeration - I agree that it is likely to be the common case.
I'm less sure about likelihood of the web app class loader returning an
empty enumeration. Absent any hard data, treating it the same way seems
reasonable.
I still don't think we should be handling nulls though.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Workaround for misbehaving ClassLoader
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 5/4/23 04:09, Mark Thomas wrote:
> On 03/05/2023 20:02, Christopher Schultz wrote:
>
> <snip/>
>
>> But my question is whether or not this is something that Tomcat should
>> be working-around. IMO the parent ClassLoader is buggy and should be
>> fixed, but it may be difficult or impossible to fix the parent, so it
>> may be worth it.
>>
>> We could even log it including the class name of the offending
>> ClassLoader.
>>
>> WDYT?
>
> The general approach we have taken is that we don't work-around bugs in
> third-party products unless:
> - the third-party vendor is (known to be) slow to respond to bugs
> - there is no other viable workaround (including switching vendors)
> - the bug impacts a reasonable proportion of Tomcat users
>
> The complexity of the workaround in Tomcat vs the severity of the issue
> is also a consideration.
>
> We also want to encourage adherence to the relevant specifications.
>
> All of the above is subjective.
>
> For commercial software, the general idea is to encourage users to put
> pressure on vendors to fix the bugs rather than expect us to - just
> because we are more responsive. This is especially true for commercial
> organizations using commercial software where there should be a support
> contract in place.
>
> For open source software, the general idea is to encourage users to
> engage with the project concerned. Open a bug, provide a PR, contribute
> and support that project.
>
> The GitHub project in question hasn't had any activity since 2017 and
> the GitHub organization hasn't had any activity since 2019. There are no
> forks of the project.
>
> It looks like the code has never been released so I am assuming the OP
> has compiled it locally.
>
> The fix in Tomcat is simple, but so is the fix in the problematic library.
>
> I think the initial position is that the OP needs to try and get this
> fixed. Whether that means creating a fork (it is ALv2 so that is easy),
> seeing if the CodeGerm team can be persuaded to accept a PR, finding an
> alternative library or something else is up to the OP.
>
> Given the options the OP has for addressing this - including a (private)
> fork, I don't think this is something that should be fixed in Tomcat.
Fair enough.
I do think there is scope for slight optimization in this area as well.
My proposed patch was something like:
if(null == parentResources) {
return childResouces;
} else if(parentFirst) {
return new CombinedEnumeration(parentResources, childResources);
} else {
return new CombinedEnumeration(childResources, parentResources);
}
A simple change of:
if(null == parentResources || !parentResources.hasMoreElements()) {
return childResouces;
} ...
means fewer wrapper objects and code executing in the common case (the
webapp probably provides most of the interesting resources to itself,
and not from the parent classloader).
In the cases where we construct a new CombinedEnumeration, I would even
say that we should check to see if either enumeration is empty and try
not to create a new object unless it's even necessary.
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Workaround for misbehaving ClassLoader
Posted by Mark Thomas <ma...@apache.org>.
On 03/05/2023 20:02, Christopher Schultz wrote:
<snip/>
> But my question is whether or not this is something that Tomcat should
> be working-around. IMO the parent ClassLoader is buggy and should be
> fixed, but it may be difficult or impossible to fix the parent, so it
> may be worth it.
>
> We could even log it including the class name of the offending ClassLoader.
>
> WDYT?
The general approach we have taken is that we don't work-around bugs in
third-party products unless:
- the third-party vendor is (known to be) slow to respond to bugs
- there is no other viable workaround (including switching vendors)
- the bug impacts a reasonable proportion of Tomcat users
The complexity of the workaround in Tomcat vs the severity of the issue
is also a consideration.
We also want to encourage adherence to the relevant specifications.
All of the above is subjective.
For commercial software, the general idea is to encourage users to put
pressure on vendors to fix the bugs rather than expect us to - just
because we are more responsive. This is especially true for commercial
organizations using commercial software where there should be a support
contract in place.
For open source software, the general idea is to encourage users to
engage with the project concerned. Open a bug, provide a PR, contribute
and support that project.
The GitHub project in question hasn't had any activity since 2017 and
the GitHub organization hasn't had any activity since 2019. There are no
forks of the project.
It looks like the code has never been released so I am assuming the OP
has compiled it locally.
The fix in Tomcat is simple, but so is the fix in the problematic library.
I think the initial position is that the OP needs to try and get this
fixed. Whether that means creating a fork (it is ALv2 so that is easy),
seeing if the CodeGerm team can be persuaded to accept a PR, finding an
alternative library or something else is up to the OP.
Given the options the OP has for addressing this - including a (private)
fork, I don't think this is something that should be fixed in Tomcat.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Workaround for misbehaving ClassLoader
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hi Chris,
Fixing the parent in this project is quite easy, if the OSGi
bundle.getResources returns null then return emptyEnumeration instead of
the direct result of the call but generally speaking this is a valid
statement but I guess Tomcat can await some other hit of this issue to push
forward a fix it since this one is very trivial to do in the failing
project ([1]).
[1]
https://github.com/CodeGerm/osgi-server/blob/master/plugins/org.cg.dao.webcontainer/src/org/cg/dao/webcontainer/tomcat/DaoClassLoader.java#L132
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Le mer. 3 mai 2023 à 21:02, Christopher Schultz <
chris@christopherschultz.net> a écrit :
> All,
>
> Please check this thread for full details:
> https://lists.apache.org/thread/of5ozg43zyk729cg311dktjcv3swct26
>
> Briefly, a user reported this NPE:
>
> Apr 29, 2023 5:41:32 PM org.apache.catalina.core.StandardContext
> listenerStart
> SEVERE: Exception sending context initialized event to listener instance
> of class [org.springframework.web.context.ContextLoaderListener]
> java.lang.NullPointerException
> at
>
> org.apache.catalina.loader.WebappClassLoaderBase$CombinedEnumeration.inc(WebappClassLoaderBase.java:2775)
> at
>
> org.apache.catalina.loader.WebappClassLoaderBase$CombinedEnumeration.hasMoreElements(WebappClassLoaderBase.java:2760)
> at
>
> org.apache.commons.logging.LogFactory.getConfigurationFile(LogFactory.java:1366)
> at
> org.apache.commons.logging.LogFactory.getFactory(LogFactory.java:453)
> at org.apache.commons.logging.LogFactory.getLog(LogFactory.java:655)
>
> Mark introduced the CombinedEnumeration in
>
> https://github.com/apache/tomcat/commit/fdd35009ead6365cbae84008e63356d6ab88ed40
> to merge the resources from the parent ClassLoader with the
> WebappClassLoader(Base). His code looks sound to me, but it can expose
> what I think of as a bug in the ClassLoader being used as the parent.
>
> ClassLoader.getResources is documented to return an empty Enumeration if
> there are no matching resources, but in this case, the ClassLoader is
> returning null.
>
> WebappClassLoaderBase assumes that the parent ClassLoader is following
> the rules and so grabs the resources from both itself and the parent and
> combines them using CombinedEnumeration.
>
> Here is a patch that will fix that:
>
> diff --git a/java/org/apache/catalina/loader/WebappClassLoaderBase.java
> b/java/org/apache/catalina/loader/WebappClassLoaderBase.java
> index aa747a5873..6f98f6e413 100644
> --- a/java/org/apache/catalina/loader/WebappClassLoaderBase.java
> +++ b/java/org/apache/catalina/loader/WebappClassLoaderBase.java
> @@ -1121,7 +1121,9 @@ public abstract class WebappClassLoaderBase
> extends URLClassLoader
> // Enumerations are combined depends on how delegation is
> configured
> boolean delegateFirst = delegate || filter(name, false);
>
> - if (delegateFirst) {
> + if(null == parentResources) {
> + return localResources;
> + } else if (delegateFirst) {
> return new CombinedEnumeration(parentResources,
> localResources);
> } else {
> return new CombinedEnumeration(localResources,
> parentResources);
>
> But my question is whether or not this is something that Tomcat should
> be working-around. IMO the parent ClassLoader is buggy and should be
> fixed, but it may be difficult or impossible to fix the parent, so it
> may be worth it.
>
> We could even log it including the class name of the offending ClassLoader.
>
> WDYT?
>
> -chris
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>