views:

2559

answers:

4

This is a follow up to my earlier question.

Tomcat 5.0.28 had a bug where the Servlet's destroy() method was not being invoked by the container on a shutdown. This is fixed in Tomcat 5.0.30, but if the Servlet's destroy() method had a System.exit(), it would result in the Tomcat windows service throwing the Error 1053 and refusing to shutdown gracefully (see above link for more details on this error)

Anybody has any idea on whether:

  • Calling System.exit() inside a Servlet's destroy() method to forcefully kill any non-daemon threads is a good idea?

  • Why does Tomcat 5.0.30 and (later versions including Tomcat 6.x.x) fail to shutdown properly if there's a System.exit() in the destroy() method of the Servlet.

+12  A: 

Calling System.exit() inside a Servlet's destroy() method to forcefully kill any non-daemon threads is a good idea?

It is absolutely not a good idea - it is a horrible idea. The destroy() method is called when the servlet is taken out of service, which can happen for any number of reasons: the servlet/webapp has been stopped, the webapp is being undeployed, the webapp is being restarted etc.

System.exit() shuts down the entire JVM! Why would you want to forcibly shutdown the entire server simply because one servlet is being unloaded?

Why does Tomcat 5.0.30 and (later versions including Tomcat 6.x.x) fail to shutdown properly if there's a System.exit() in the destroy() method of the Servlet.

Probably to prevent such dangerous behavior like this.

You shouldn't write code that assumes that your code/application is the only thing running on the server.

matt b
Well, I'm maintaining this code. Also, its pretty guaranteed that this will be the only web application running on this Tomcat.
Nikhil Kashyap
+1, System.exit() should NEVER be called in a servlet under ANY circumstances. Period.
David Zaslavsky
@Nikhil - that doesn't explain WHY you are doing this. You asked if it's a good idea, and the answer is that it is a bad practice. Perhaps if you can tell us what you are attempting to accomplish we can help you find a better solution?
matt b
In other words, this "Error 1053" is a side effect of a bad practice, not the true cause of your problems
matt b
See my answer below for a complete best-practice pattern for managing threads started by a servlet.
Jared
+3  A: 

Calling System.exit() inside a Servlet's destroy() method to forcefully kill any non-daemon threads is a good idea?

Not a good idea. You will forcefully kill all threads, which might include part of Tomcat that is currently shutting down the system. This will cause Tomcat to un-gracefully shutdown. This can also prevent shutdown handlers from running which can lead to all sorts of problems.

Why does Tomcat 5.0.30 and (later versions including Tomcat 6.x.x) fail to shutdown properly if there's a System.exit() in the destroy() method of the Servlet.

A lot of code executes after a Servlet destory. The Context destroy and all of its other listeners for one... other servlets. Other applications. Tomcat itelf. By calling System.exit, you prevent all of that from running.

A better question is what are thse non-daemon threads, why are they running, and who starts them?

James Schek
To answer your last question, there are lots of threads that do various tasks like polling, reporting and so on. The servlet initializes these threads and then they start running.
Nikhil Kashyap
+5  A: 

You're asking two questions:

Question 1: Is calling System.exit() inside a Servlet's destroy() method to forcefully kill any non-daemon threads a good idea?

Calling System.exit() inside ANY servlet-related method is always 100% incorrect. Your code is not the only code running in the JVM - even if you are the only servlet running (the servlet container has resources it will need to cleanup when the JVM really exits.)

The correct way to handle this case is to clean up your threads in the destroy() method. This means starting them in a way that lets you gently stop them in a correct way. Here is an example (where MyThread is one of your threads, and extends ServletManagedThread):

 public class MyServlet extends HttpServlet {
    private List<ServletManagedThread> threads = new ArrayList<ServletManagedThread>();     

     // lots of irrelevant stuff left out for brevity

    public void init() {
        ServletManagedThread t = new MyThread();
        threads.add(t);
        t.start();
    }

    public void destroy() {
        for(ServletManagedThread thread : threads) {
           thread.stopExecuting();
        }
    }
 }

 public abstract class ServletManagedThread extends Thread {

    private boolean keepGoing = true;

    protected abstract void doSomeStuff();
    protected abstract void probablySleepForABit();
    protected abstract void cleanup();

    public void stopExecuting() {
       keepRunning = false;
    }

    public void run() {
       while(keepGoing) {
            doSomeStuff();
            probablySleepForABit();
       }
       this.cleanup();
    }
}

It's also worth noting that there are thread/concurrency libraries out there that can help with this - but if you really do have a handful of threads that are started at servlet initialization and should run until the servlet is destroyed, this is probably all you need.

Question 2: Why does Tomcat 5.0.30 and (later versions including Tomcat 6.x.x) fail to shutdown properly if there's a System.exit() in the destroy() method of the Servlet?

Without more analysis, it's hard to know for certain. Microsoft says that Error 1053 occurs when Windows asks a service to shutdown, but the request times out. That would make it seem like something happened internally to Tomcat that got it into a really bad state. I would certainly suspect that your call to System.exit() could be the culprit. Tomcat (specifically, Catalina) does register a shutdown hook with the VM (see org.apache.catalina.startup.Catalina.start(), at least in 5.0.30). That shutdown hook would get called by the JVM when you call System.exit(). The shutdown hook delegates to the running services, so each service could potentially be required to do alot of work.

If the shutdown hooks (triggered by your System.exit()) fail to execute (they deadlock or something like that,) then it is very easy to understand why the Error 1053 occurs, given the documentation of the Runtime.exit(int) method (which is called from System.exit()):

If this method is invoked after the virtual machine has begun its shutdown sequence then if shutdown hooks are being run this method will block indefinitely. If shutdown hooks have already been run and on-exit finalization has been enabled then this method halts the virtual machine with the given status code if the status is nonzero; otherwise, it blocks indefinitely.

This "indefinite blocking" behavior would definitely cause an Error 1053.

If you want a more complete answer than this, you can download the source and debug it yourself.

But, I would be willing to bet that if you properly handle your thread management issue (as outlined above,) your problems will go away.

In short, leave the System.exit() call to Tomcat - that's not your job.

Jared
I'd go so far as to say 99.99% of all apps never need to call System.exit(). Just break out of the loop in your main method or return from your thread's run() method.
Mr. Shiny and New
Agreed, Mr. Shiny. Anytime you see a call to System.exit(), you should suspect poor program design.
Jared
I believe keepGoing should be marked volatile (or its accesses be synchronized), otherwise some threads might not see the new value.
Jerome
@Jared System.exit(int) is required if you want to provide an exit status for your program, like basically all command line tools do.
Geoff
@Geoff - Yep. If you're writing a command line utility that needs to manipulate the exit code, you need to call System.exit(). If you need to manipulate the exit code from a servlet-related process, I'd seriously question your system design. System.exit() is only useful in situations where you have very tight control over the whole system - servlets are not that kind of situation - someone else (the container) is in charge of the system, not your servlet.
Jared
A: 

When writing thread shutdown code like Jared's, I normally make the "keepGoing" member and "stopExecuting()" method static so that all threads get the signal to go down with one shutdown call. Good idea or no?

I have a deep-inbuilt aversion to all things static - especially when dealing with concurrency. That's not to say that approach is wrong or that it won't work or that it's not more efficient. My aversion to static is probably excessive.
Jared
Actually...in practice, I would usually implement MyThread as an abstract class, with doSomeStuff(), probablySleepForABit() and cleanup() all being abstract...because statics are not inherited, that wouldn't work.
Jared
Edited my answer to include the abstraction of "servlet thread management."
Jared
I'd keep the keepGoing var non-static because sometimes you want to shut down just one thread. It's easy enough to put all your thread handles in one collection and just iterate over it and stop the threads.
Mr. Shiny and New