[Java] How to shut my Thread down nicely?



  • Hi there,

    me and a friend are sitting at work and just can't grasp the following:

    I got a HttpServlet which is started on webapp launch and then starts my Thread, on its destroy-method the Servlet should call myThread.interrupt(); to clean things up.

    The Thread itself is doing a DB-query periodically (don't ask, the whole webapp is a major WTF with tons of batch files, servlets, harcoded passwords, cleartext passwords in the db... you get the picture), so it has a connection to the database. If it gets interrupted i'd like to close the connection and shut down nicely.

    Now i realise you normally would code a bunch of listeners, events, queus, etc. but its a single thread in an wtf environment.

    So to get to the point: Would this work?

    package xx.xxxx.periodic;

    public class xxxThread extends Thread {

        long intervall;
       
        public xxxThread(long intervall) {
            this.intervall = intervall;
        }

        public void run() {
           
            if(isInterrupted()) {
                //Cleanup
                //Major WTF?
            }
           
            while(!isInterrupted()) {
                //DO STUFF
               
                try {
                    sleep(intervall);
                } catch (InterruptedException ir) {
                    //Cleanup
                }
            }
           
        }
    }
     

    Would that work? Or how would you do it?

    Im not that fit in Threading and i realise that this might be very bad style, or even not working; but thats why I'm asking you.

    Regards,

    nFec 



  • The better thing to do is change the while condition to while(true), and put the while loop inside the try block.  That way, the first interrupt will terminate the loop simply by transferring control to the catch block.

    As you have it, the while loop probably will never exit, because the occurrence of an InterruptedException never results in isInterrupted() returning true.  So the only way isInterrupted() can ever return true is if the interruption occurs after the call to sleep but before the next call to isInterrupted, which is a pretty slim window.



  • [quote user="nFec"]

    package xx.xxxx.periodic;

    public class xxxThread extends Thread {

        long intervall;
       
        public xxxThread(long intervall) {
            this.intervall = intervall;
        }

        public void run() {
           
            if(isInterrupted()) {
                //Cleanup
                //Major WTF?
            }
           
            while(!isInterrupted()) {
                //DO STUFF
               
                try {
                    sleep(intervall);
                } catch (InterruptedException ir) {
                    //Cleanup
                }
            }
           
        }
    }
    [/quote]
     
    Get rid of the first check for interruption, move all cleanup to after the while loop, and put a break statement in the catch block. This way cleanup will happen no matter when the interruption occurs.



  • [quote user="endergt"]

    Get rid of the first check for interruption, move all cleanup to after the while loop, and put a break statement in the catch block. This way cleanup will happen no matter when the interruption occurs.

    [/quote]

     Edit timer ran out. Your new code would look like:

     package xx.xxxx.periodic;

    public class xxxThread extends Thread {

        long intervall;
       
        public xxxThread(long intervall) {
            this.intervall = intervall;
        }

        public void run() {

            while(!isInterrupted()) {
                //DO STUFF
               
                try {
                    sleep(intervall);
                } catch (InterruptedException ir) {
                    break;
                }
            }
            // Cleanup
        }
    }
     
    Notice there's no isInterrupted() conditional around cleanup - it should always happen.



  • Why not use somehing simple along the lines of 

     

     

    package xx.xxxx.periodic;
    
    public class xxxThread implements Runnable { // IMO implementing Runnable is more beautiful than extending Thread
    
      long intervall;
    
      private volatile finished = false;
    
      public void setFinished(finished) {
        this.finished = finished;
      }
    
      public xxxThread(long intervall) {
        this.intervall = intervall;
      }
    
      public void run() {
        while(!finished) {
          //DO STUFF
          try {
            sleep(intervall);
          }
          catch (InterruptedException ir) {
            //ignore
          }
        }
        //Cleanup
      }
    }
    


    So the Servlet would call thread.setFinished(true). Plus you can use Thread.interrupt() for more meaningfull purposes, e.g. interrupt the sleep() to make the thread work again.


  • Thread interrupts are generally at the mercy of the host operating system.

    Here is what Java is *supposed* to do:

    1.  If the thread is just executing normal code, the interrupt status is set and nothing will happen unless you check Thread.interrupted() (which clears the interrupt status) or Thread.isInterrupted() (which does not clear the interrupt status).

    2.  If the thread is executing Object.wait(), Object.wait(long), Object.wait(long, int), Thread.join(), Thread.join(long), Thread.join(long, int), Thread.sleep(long) or Thread.sleep(long, int), the thread's interrupt status will be cleared (not set like you'd expect) and the thread will get an InterruptedException.  (If you're using sleep(long) only to test the interrupt status, you could do the same with if(interrupted()) throw new InterruptedException();).

    3.  If the thread is blocked on an interruptible channel, the channel will be closed, the interrupt status will be set, and the thread will get a ClosedByInterruptException.

    4.  If the thread is blocked on a Selector, it will return from the selection operation with the interrupt status set.

    Your connection to the DB may be on a Selector or on an interruptible channel, or who knows what else.

    I've found, however, that in some cases an interruptible channel does not interrupt -- if there's something in the underlying operating system (YES WINDOWS 2000 I'M TALKING ABOUT YOU) that won't break the connection, Java can't fix it.



  • [quote user="newfweiler"]

    3.  If the thread is blocked on an interruptible channel, the channel will be closed, the interrupt status will be set, and the thread will get a ClosedByInterruptException.

    4.  If the thread is blocked on a Selector, it will return from the selection operation with the interrupt status set.

    [/quote]

    For those reasons, I would not use Thread.interrupt() unless I really really have to or I know for sure that the thread is in sleep() or wait().

    Because I don't know (and don't want to learn the hard way) how a JDBC driver or something similar behaves when being interrupted... It might wrack havoc to the database connection, so the cleanup cannot be done.
     



  • [quote user="ammoQ"]

    Why not use somehing simple along the lines of 

     

     

    package xx.xxxx.periodic;
    
    public class xxxThread implements Runnable { // IMO implementing Runnable is more beautiful than extending Thread
    
      long intervall;
    
      private volatile finished = false;
    
      public void setFinished(finished) {
        this.finished = finished;
      }
    
      public xxxThread(long intervall) {
        this.intervall = intervall;
      }
    
      public void run() {
        while(!finished) {
          //DO STUFF
          try {
            sleep(intervall);
          }
          catch (InterruptedException ir) {
            //ignore
          }
        }
        //Cleanup
      }
    }
    


    So the Servlet would call thread.setFinished(true). Plus you can use Thread.interrupt() for more meaningfull purposes, e.g. interrupt the sleep() to make the thread work again.
    [/quote]

    I think you should at least handle the exception and not just ignore it. All this does is hide an error condition.


  • Thanks for your help!

     I'll try the catch... break; and cleanup after the loop thingie.

    @ extending Thread vs. implement Runnable:

    I personally don't like to implement Runnable cause this is ugly imho: Thread t = new Thread(myRunnableObject); t.start();
    I'd only implement Runnable if i had to extend from another class too.

    But thats not the point here ;)

     

    Regards

    nFec 

     

     



  • [quote user="ammoQ"]

    For those reasons, I would not use Thread.interrupt() unless I really really have to or I know for sure that the thread is in sleep() or wait().

    Because I don't know (and don't want to learn the hard way) how a JDBC driver or something similar behaves when being interrupted... It might wrack havoc to the database connection, so the cleanup cannot be done.
     

    [/quote]

     

    Well but i need to interrupt it, because if the Servlet gets destroyed that means, the Webserver is going down; and i need to clean up and stop my work then. 



  • Implement Runnable.

    No, really. Extending java.lang.Thread is the wrong thing to do.  Your code is not a thread, your code runs on a thread, so it's a bad abstraction.  It also makes it harder (not in and of itself *hard*, but harder) to change the code to use a threadpool (which take Runnable, or more recently Future, which can wrap Runnable via the builtin FutureTask class).

    To the OP: if you're using Thread#interrupt() for process control, make sure your exception handling is 100% perfect.  Eventually an InterruptedException will arrive in the middle of, say, an IO operation performed by the JDBC driver, which will turn into an unexpected and completely unpredictable SQLException (if you're lucky) or RuntimeException (if you're not).  It's not just sleep and wait that are interruptable.

    You're better off using a flag, or a timer (look into Quartz for scheduling repetitive tasks outside an app server but inside the application, or just use java.util.Timer for simple stuff) rather than managing threads directly from a servlet.

    I'd also seriously consider fixing the underlying problem rather than bandaging it over with a thread that polls the database.  I mean, seriously, wtf? 



  • Yeah i know, like stated the whole project is (except its goal) a major WTF.

    But i got no saying in this, i even DROPPED the production database (after a backup) via SQL injection FROM HOME.
    I pointed out how it worked etc. first my boss was stunned, cause he got no clue, then he went angry and then he told me: "Well youre an expert, nobody else will know how to do this."
    Well i could rant about it all day, but the thing is, i need to finish this assignment; it might be ugly but i dont want to produce new problems.

    So what youre basically saying is to put a try. catch around the whole execution code?
    Shouldn't be a problem, cause this only "logs out" users, what is only checked for displaying rather than security issues.

    Yeh WTF again...



  • [quote user="pokari"][quote user="ammoQ"]

    Why not use somehing simple along the lines of 

    (snip)

          try {
    sleep(intervall);
    }
    catch (InterruptedException ir) {
    //ignore
    }
    [/quote]

    I think you should at least handle the exception and not just ignore it. All this does is hide an error condition.

    [/quote]

    I fail to see how being interrupted during sleep could be considered an "error condition"...
     



  • [quote user="nFec"][quote user="ammoQ"]

    For those reasons, I would not use Thread.interrupt() unless I really really have to or I know for sure that the thread is in sleep() or wait().

    Because I don't know (and don't want to learn the hard way) how a JDBC driver or something similar behaves when being interrupted... It might wrack havoc to the database connection, so the cleanup cannot be done.

    [/quote]

    Well but i need to interrupt it, because if the Servlet gets destroyed that means, the Webserver is going down; and i need to clean up and stop my work then. 

    [/quote]

    If you interrupt the JDBC driver, chances are you won't be able to do any cleanup at all. What you could do is to set a boolean volatile(!) variable "sleeping" to true before going to sleep, reset it to false afterwards and check it in setFinished; if it is true, interrupt the thread.

    Anyway, if your cleanup takes some time, but the webserver is going down, I can not see how you could guarantee that cleanup finishes.
     



  • [quote user="ammoQ"]If you interrupt the JDBC driver, chances are you won't be able to do any cleanup at all.[/quote]

    AmmoQ, may his code pass all tests, speaks truth: if interrupt() arrives while or immediately before (remember, thread interrupts are only delivered at specific points) the JDBC driver is doing IO, and the programmer who wrote your JDBC driver is as lazy pressed for time as the rest of us, it'll simply treat its connection to the database as fubar-ed and stop even trying to communicate.  And because it's a thread synchronization issue, you'll have a merry hell of a time tracking this down.

    You mention this is just to "clean up" user sessions in the database -- what about adding a session lifecycle listener to your web container and cleaning up each user's data as their session expires, then cleaning up any remaining data during servlet shutdown?  Then you just have to make sure the last thing you do is close the JDBC connection.  If you're using a container-managed data source rather than connecting to the database yourself (and I'm pretty sure even standalone Tomcat can deal with this) then even that step will be handled for you.


Log in to reply