Bug 56387 - Improve exceptions thrown by stopped WebappClassLoader (to help identify 'stopped state' as the cause of a NoClassDefFoundError)
Summary: Improve exceptions thrown by stopped WebappClassLoader (to help identify 'sto...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.0.x-trunk
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-11 07:59 UTC by qiuboboy
Modified: 2014-05-19 19:38 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description qiuboboy 2014-04-11 07:59:51 UTC
org.apache.catalina.loader.WebappClassLoader#started field isn't volatile,when org.apache.catalina.loader.WebappClassLoader#stop is executing by thread A,and  thread  B is loading class, thread b could see non-updated value started=false. and then  cause a NoClassDefFoundError.
Comment 1 Konstantin Kolinko 2014-04-11 15:19:39 UTC
1. You are confusing 'false' and 'true'. It is just wrong.

2. At best it is just a theory. Can you demonstrate it with a testcase?

3. When webapp stops, all its processing threads should have already been stopped. There are plenty of syncs involved when that happens, thus volatility is not an issue.
Comment 2 qiuboboy 2014-04-14 01:37:26 UTC
yes,thread b could see non-updated value started=true,my typo.

we found this excepiton:

java.lang.NoClassDefFoundError: org/jboss/netty/util/internal/ExecutorUtil
        at org.jboss.netty.channel.socket.nio.NioServerSocketChannelFactory.releaseExternalResources(NioServerSocketChannelFactory.java:146) ~[netty-3.2.5.Final.jar:na]
        at org.jboss.netty.bootstrap.Bootstrap.releaseExternalResources(Bootstrap.java:324) ~[netty-3.2.5.Final.jar:na]
        at com.alibaba.dubbo.remoting.transport.netty.NettyServer.doClose(NettyServer.java:124) ~[dubbo-2.5.3.jar:2.5.3]
        at com.alibaba.dubbo.remoting.transport.AbstractServer.close(AbstractServer.java:155) [dubbo-2.5.3.jar:2.5.3]
        at com.alibaba.dubbo.remoting.transport.AbstractServer.close(AbstractServer.java:163) [dubbo-2.5.3.jar:2.5.3]
        at com.alibaba.dubbo.remoting.exchange.support.header.HeaderExchangeServer.close(HeaderExchangeServer.java:121) [dubbo-2.5.3.jar:2.5.3]
        at com.alibaba.dubbo.rpc.protocol.dubbo.DubboProtocol.destroy(DubboProtocol.java:395) [dubbo-2.5.3.jar:2.5.3]
        at com.alibaba.dubbo.rpc.protocol.ProtocolFilterWrapper.destroy(ProtocolFilterWrapper.java:66) [dubbo-2.5.3.jar:2.5.3]
        at com.alibaba.dubbo.rpc.protocol.ProtocolListenerWrapper.destroy(ProtocolListenerWrapper.java:72) [dubbo-2.5.3.jar:2.5.3]
        at com.alibaba.dubbo.config.ProtocolConfig.destroyAll(ProtocolConfig.java:435) [dubbo-2.5.3.jar:2.5.3]
        at com.alibaba.dubbo.config.AbstractConfig$1.run(AbstractConfig.java:452) [dubbo-2.5.3.jar:2.5.3]
        at java.lang.Thread.run(Thread.java:722) [na:1.7.0_03]
Caused by: java.lang.ClassNotFoundException: org.jboss.netty.util.internal.ExecutorUtil
        at org.apache.catalina.loader.WebappClassLoader.loadClass(WebappClassLoader.java:1711) ~[na:na]
        at org.apache.catalina.loader.WebappClassLoader.loadClass(WebappClassLoader.java:1556) ~[na:na]
        ... 12 common frames omitted

the thread b is a java shunt down hook.so,it haven't been stopped .
Comment 3 Konstantin Kolinko 2014-04-14 08:06:34 UTC
It is your responsibility to stop any threads that a web application may have started.  Especially, an application (and libraries that it uses) must not register any shutdown hooks.
Comment 4 qiuboboy 2014-04-14 15:11:32 UTC
Thanks your advice, Konstantin.

Some libraries not only run in tomcat,it can be used in standalone application, shutdown hook is a ideal place to close external resources.I think tomcat must be throw a reasonable exception such as IllegalStateException("tomcat has been stopped.") to hint users to handle their misusage.

Add volatile keyword to org.apache.catalina.loader.WebappClassLoader#started can help us to find problem quickly.

(In reply to Konstantin Kolinko from comment #3)
> It is your responsibility to stop any threads that a web application may
> have started.  Especially, an application (and libraries that it uses) must
> not register any shutdown hooks.
Comment 5 Konstantin Kolinko 2014-04-14 18:14:42 UTC
1. What exactly is your version of Tomcat?

So that I can interpret those line numbers in the exception.

2. I agree that the code could be improved to provide more detail.

a) It cannot throw an IllegalStateException (per the API contract), but it can nest one into ClassNotFoundException that it throws.

b) In WebappClassLoader.loadClass(String, boolean)

        if (!started) {
            try {
                throw new IllegalStateException();
            } catch (IllegalStateException e) {
                log.info(sm.getString("webappClassLoader.stopped", name), e);
            }
        }

That is some logic that logs the access. It would be better to follow that by failing fast via throwing an ClassNotFoundException instead of continuing.

c) In WebappClassLoader.findClass(String)

It fails fast, but does not provide any details, nor logs this access.

        // Cannot load anything from local repositories if class loader is stopped
        if (!started) {
            throw new ClassNotFoundException(name);
        }


> Add volatile keyword to org.apache.catalina.loader.WebappClassLoader#started
> can help us to find problem quickly.

That would not hurt, but I do not believe that that would help you. I do not believe that your thread saw stale value of that flag.


I am REOPENING this issue and changing severity and title. I do not see a bug here, but the behaviour can be improved.
Comment 6 qiuboboy 2014-04-15 07:53:05 UTC
(In reply to Konstantin Kolinko from comment #5)
> 1. What exactly is your version of Tomcat?
> 
> So that I can interpret those line numbers in the exception.

we use tomcat 7.0.27.

> 2. I agree that the code could be improved to provide more detail.
> 
> a) It cannot throw an IllegalStateException (per the API contract), but it
> can nest one into ClassNotFoundException that it throws.

I agree with you idea,nest one into ClassNotFoundException will be prefect.

> b) In WebappClassLoader.loadClass(String, boolean)
> 
>         if (!started) {
>             try {
>                 throw new IllegalStateException();
>             } catch (IllegalStateException e) {
>                 log.info(sm.getString("webappClassLoader.stopped", name), e);
>             }
>         }
> 
> That is some logic that logs the access. It would be better to follow that
> by failing fast via throwing an ClassNotFoundException instead of continuing.
> 
> c) In WebappClassLoader.findClass(String)
> 
> It fails fast, but does not provide any details, nor logs this access.
> 
>         // Cannot load anything from local repositories if class loader is
> stopped
>         if (!started) {
>             throw new ClassNotFoundException(name);
>         }
> 
> 
> > Add volatile keyword to org.apache.catalina.loader.WebappClassLoader#started
> > can help us to find problem quickly.
> 
> That would not hurt, but I do not believe that that would help you. I do not
> believe that your thread saw stale value of that flag.

NoClassDefFoundError happen Occasionally.and I analyze the code,only thread see stale value of that flag will cause the exception.
 
> I am REOPENING this issue and changing severity and title. I do not see a
> bug here, but the behaviour can be improved.

thanks your help.^_^
Comment 7 Mark Thomas 2014-05-19 19:38:06 UTC
I agree that no evidence has been produced that supports the assertation that a state value of the started flag was observed. Note the state is volatile in Tomcat 8.

I have improved the code that handles an attempt to load a class after a web application has been stopped. This will be in 8.0.9 onwards.