Bug 37264

Summary: ServletContextListener cannot load classes
Product: Tomcat 5 Reporter: Bogdan Calmac <bc4all>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: normal    
Priority: P2    
Version: Nightly Build   
Target Milestone: ---   
Hardware: All   
OS: other   
Attachments: StandardContext patch

Description Bogdan Calmac 2005-10-27 06:48:24 UTC
If a ServletContextListener implmentation references a class that is not yet 
loaded, the loading would with some ZipException.

This is caused by the fact that when a context is stopped, the classloader is 
unbound before the ServletContextListeners are called. The sequence of calls is 
something like this:

StandardContext.stop()
  lifecycle.fireLifecycleEvent(STOP_EVENT, null);
    NamingContextListener.lifecycleEvent();
      ContextBindings.unbindClassLoader()
  ...
  listenerStop(); // trouble

My solutions was to move listenerStop() before the call to fireLifecycleEvents()


I also filed bug 37262, about the datasources not being released when the 
context is stopped. These two are somehow related, I think the right sequence 
when stopping the context is:
1. call the listeners
2. release the datasources
3. unbind the classloader
Comment 1 Bogdan Calmac 2005-10-27 06:48:54 UTC
Created attachment 16816 [details]
StandardContext patch
Comment 2 Bogdan Calmac 2005-10-27 06:52:28 UTC
A necessary clarification: This problem refers to ServletContextlistener.
destroy()
Comment 3 Darryl Miles 2005-10-27 09:28:14 UTC
ServletContextlistener.destroy() ???  or ServletContextlistener.contextDestroyed()


What about ServletContextlistener.contextInitialized() and listenerStart(),
would it be possible to review that too with your knowledge of whats going on.

I suspect the calling sequences for all setup/teardown should be Last In, First Out.

If your solution is verified to be correct, I think you might have just
possibily found the root cause of a great many outstanding ThreadDeath related
issues that have been bounced around on bugzilla recently.  These are
ThreadDeath's caused from listenerStop() execution where the class loader has
been invalidated.
Comment 4 Remy Maucherat 2005-10-27 10:10:57 UTC
(In reply to comment #3)
> ServletContextlistener.destroy() ???  or ServletContextlistener.contextDestroyed()
> 
> 
> What about ServletContextlistener.contextInitialized() and listenerStart(),
> would it be possible to review that too with your knowledge of whats going on.
> 
> I suspect the calling sequences for all setup/teardown should be Last In,
First Out.
> 
> If your solution is verified to be correct, I think you might have just
> possibily found the root cause of a great many outstanding ThreadDeath related
> issues that have been bounced around on bugzilla recently.  These are
> ThreadDeath's caused from listenerStop() execution where the class loader has
> been invalidated.

Understanding things is good rather than simply writing BS. This is related to
JNDI. The code as of right now is:

            // Stop our application listeners
            listenerStop();

            // Clear all application-originated servlet context attributes
            if (context != null)
                context.clearAttributes();

            // Stop resources
            resourcesStop();

            if ((realm != null) && (realm instanceof Lifecycle)) {
                ((Lifecycle) realm).stop();
            }
            if ((cluster != null) && (cluster instanceof Lifecycle)) {
                ((Lifecycle) cluster).stop();
            }
            if ((logger != null) && (logger instanceof Lifecycle)) {
                ((Lifecycle) logger).stop();
            }
            if ((loader != null) && (loader instanceof Lifecycle)) {
                ((Lifecycle) loader).stop();
            }


Comment 5 Remy Maucherat 2005-10-27 10:15:57 UTC
I don't like your patch, but NamingContextListener could be adjusted to do its
stuff in AFTER_STOP_EVENT, so that JNDI still works when stopping the listeners.
If you test that and if it works, I can make the change.
Comment 6 Bogdan Calmac 2005-10-27 16:02:40 UTC
I don't like the patch either, it is indeed more appropiate to move some code
from STOP_EVENT to AFTER_STOP_EVENT. I'll test the patch and post the results.
With my limited knowledge of the code base, I think the fix involves moving this
whole block from STOP to AFTER_STOP:

            if (container instanceof Context) {
                ContextBindings.unbindClassLoader
                    (container, container, 
                     ((Container) container).getLoader().getClassLoader());
            }

            if (container instanceof Server) {
                namingResources.removePropertyChangeListener(this);
                ContextBindings.unbindClassLoader
                    (container, container, 
                     this.getClass().getClassLoader());
            }

            ContextAccessController.unsetSecurityToken(getName(), container);

            namingContext = null;
            envCtx = null;
            compCtx = null;
            initialized = false;
Comment 7 Remy Maucherat 2005-10-27 16:04:08 UTC
I did apply something very close to the proposed patch, but did not test it well.