In ServletWrapper.handleReload, there is a syncronization problem. If tomcat us under
a load and a servlet class file or jsp is updated, there is a potential for multiple
request to try and reload the servlet simultaneously and step on each other causing a
bad state. The way it is currently coded, we've seen cases where when a class is
updated, the first thread through checks to see if a reload needs to take place and
starts the destroy. The next thread comes in and also thinks it is supposed to do a
reload and starts down the same branch. Meanwhile, the first thread continues and
creates the new loader (loader.reload) and finishes handleReload (including setting
servlet=null and servletClass=null). The second thread then continues it's
processing, and gets to creates a new loader. While the second thread is creating the
new loader, the first thread runs the Handler.service method which runs the
ServletWrapper.init method which calls loadServlet where servlet and servletClass are
set appropriately, and eventually calls ServletWrapper.doInit which sets initialized
to true. Then the second thread continues and nulls out servlet and servletClass, but
since initialized is true, it doesn't run the init method and from the on you get a
null pointer exception trying to access that particular servlet.
I've even tried just moving the lines that null out servlet and servletClass up to
right after where initialized is set to false, but this causes another problem where
the first thread creates a new loader and calls loadServlet, then the second thread
creates a new loader but the servlet isn't reinitialized (since it doesn't need to be)
and so the loader doesn't have it cached and always reports false when its
shouldReload method is called. This means that the servlet never really gets
reloaded because the loader doesn't have it cached.
Anyways, attached is a patch that fixes this problem. I synchronized before I call
load.shouldReload so that only the first thread will actually call loader.reload with
subsequent threads getting a false back from the new loader so that they don't also
try and create a new loader or null out the class variables. I've tested this on
Windoze (Win2K) and NetWare against the Tomcat HTTP handler and via Apache/Tomcat. It
does slow performance during the actually reload, but is minimal otherwise and if the
context is set to not reload, the syncronized call is bypassed altogether. I would
like to get this in to 3.2.2 if at all possible since it does affect us heavily in our
development and testing on NetWare. Even though the diff seems large, all that was
added was the synchronized call (and it's preceding comment). The rest of the
differences are just indenting the existing code.
Mike Anderson
Senior Software Engineer
Platform Services Group
[EMAIL PROTECTED]
Novell, Inc., the leading provider of Net services software
www.novell.com
Index: ServletWrapper.java
===================================================================
RCS file:
/home/cvspublic/jakarta-tomcat/src/share/org/apache/tomcat/core/Attic/ServletWrapper.java,v
retrieving revision 1.60.2.5
diff -u -r1.60.2.5 ServletWrapper.java
--- ServletWrapper.java 2001/01/12 04:39:05 1.60.2.5
+++ ServletWrapper.java 2001/05/02 22:42:27
@@ -422,38 +422,44 @@
if( isReloadable ) {// && ! "invoker".equals( getServletName())) {
ServletLoader loader=context.getServletLoader();
if( loader!=null) {
- // XXX no need to check after we remove the old loader
- if( loader.shouldReload() ) {
- // workaround for destroy
- try {
- destroy();
- } catch(Exception ex ) {
- context.log( "Error in destroy ", ex );
- }
- initialized=false;
- loader.reload();
-
- ContextManager cm=context.getContextManager();
- cm.doReload( req, context );
-
- servlet=null;
- servletClass=null;
- /* Initial attempt to shut down the context and sessions.
-
- String path=context.getPath();
- String docBase=context.getDocBase();
- // XXX all other properties need to be saved
- // or something else
- ContextManager cm=context.getContextManager();
- cm.removeContext(path);
- Context ctx=new Context();
- ctx.setPath( path );
- ctx.setDocBase( docBase );
- cm.addContext( ctx );
- context=ctx;
- // XXX shut down context, remove sessions, etc
- */
- }
+ // We need to syncronize here so that multiple threads don't
+ // try and reload the class. The first thread through will
+ // create the new loader which will make shouldReload return
+ // false for subsequent threads.
+ synchronized(this) {
+ // XXX no need to check after we remove the old loader
+ if( loader.shouldReload() ) {
+ // workaround for destroy
+ try {
+ destroy();
+ } catch(Exception ex ) {
+ context.log( "Error in destroy ", ex );
+ }
+ initialized=false;
+ loader.reload();
+
+ ContextManager cm=context.getContextManager();
+ cm.doReload( req, context );
+
+ servlet=null;
+ servletClass=null;
+ /* Initial attempt to shut down the context and sessions.
+
+ String path=context.getPath();
+ String docBase=context.getDocBase();
+ // XXX all other properties need to be saved
+ // or something else
+ ContextManager cm=context.getContextManager();
+ cm.removeContext(path);
+ Context ctx=new Context();
+ ctx.setPath( path );
+ ctx.setDocBase( docBase );
+ cm.addContext( ctx );
+ context=ctx;
+ // XXX shut down context, remove sessions, etc
+ */
+ }
+ }
}
}
}
