I posted this a couple of days ago and haven't seen anything on it. Do I need to log a bug or just assume it's not really important to the tomcat crowd in general? Mike Anderson
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.comIndex: 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 + */ + } + } } } }
