Where to use Thread interupt

Question

I have some old code I am working with, and I'm not too experienced with Threads (mostly work on the front end). Anyway, this Thread.sleep is causing the thread to hang and I'm unsure what to do about it. I thought about using a counter and throwing a Thread.currentThread.interupt, but unsure of where to put it or which thread it will interupt. Here is an example of the dump. As you can see the thread count is getting pretty high at 1708.

Any advice?

"Thread-1708" prio=6 tid=0x2ceec400 nid=0x2018 waiting on condition [0x36cdf000] java.lang.Thread.State: TIMED_WAITING (sleeping) at java.lang.Thread.sleep(Native Method) Locked ownable synchronizers: - None "Thread-1707" prio=6 tid=0x2d16b800 nid=0x215c waiting on condition [0x36c8f000] java.lang.Thread.State: TIMED_WAITING (sleeping) at java.lang.Thread.sleep(Native Method) Locked ownable synchronizers: - None

@Override
public void run()
{
    Connection con = null;
    int i = 0;

    while (is_running)
    {
        try
        {
            con = ConnectionManager.getConnection();

            while (!stack.isEmpty())
            {
                COUNT++;
                String line = (String) stack.pop();
                getPartMfr(line);
                try
                {
                    if (this.mfr != null && !this.mfr.equals(EMPTY_STR))
                    {
                        lookupPart(con, line);
                    }
                }
                catch (SQLException e)
                {
                    e.printStackTrace();
                }
                if (COUNT % 1000 == 0)
                {
                    Log log = LogFactory.getLog(this.getClass());
                    log.info("Processing Count: " + COUNT);
                }
            }
        }
        catch (NamingException e)
        {
            e.printStackTrace();
        }
        catch (SQLException e)
        {
            e.printStackTrace();
        }
        finally
        {
            try
            {
                ConnectionManager.close(con);
            }
            catch (SQLException e)
            {
                e.printStackTrace();
            }
        }


        try {
            Thread.sleep(80);

        } catch (InterruptedException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }

    }
    this.finished = true;

}

Here is where it calls the run method, as you can see it does set it to false, but I guess it is missing threads?

    HarrisWorker w[] = new HarrisWorker[WORKER_POOL_SIZE];
    try
    {
        for (int i = 0; i < w.length; i++)
        {
            w[i] = new HarrisWorker(pw);
            w[i].start();
        }
        pw.println(headers());
        File inputDir = new File(HARRIS_BASE);
        String files[] = inputDir.list();
        for (String file : files)
        {

            try
            {
                File f = new File(HARRIS_BASE + File.separator + file);
                if (f.isDirectory())
                    continue;
                final String workFile = workDir + File.separator + file;
                f.renameTo(new File(workFile));

                FileReader fr = new FileReader(workFile);
                BufferedReader br = new BufferedReader(fr);
                String line = br.readLine();

                boolean firstLine = true;

                while (line != null)
                {
                    if (firstLine)
                    {
                        firstLine = false;
                        line = br.readLine();
                        continue;
                    }
                    if (line.startsWith(","))
                    {
                        line = br.readLine();
                        continue;
                    }
                    //          if(line.indexOf("103327-1") == -1)
                    //          {
                    //              line = br.readLine();
                    //              continue;
                    //          }
                    HarrisWorker.stack.push(line);
                    line = br.readLine();
                }
                br.close();
                fr.close();
                for (int i = 0; i < w.length; i++)
                {
                    w[i].is_running = false;

                    while (!w[i].finished)
                    {

                        Thread.sleep(80);   
                    }

                }
                move2Processed(file, workFile);
                long etime = System.currentTimeMillis();
                System.out.println("UNIQUE PARTS TOTAL FOUND: " + HarrisWorker.getFoundCount() + " of " + HarrisWorker.getUniqueCount() + ", "
                        + (HarrisWorker.getFoundCount() / HarrisWorker.getUniqueCount()));
                System.out.println("Time: " + (etime - time));


            }
            catch (Exception e)
            {
                e.printStackTrace();
                File f = new File(workDir + File.separator + file);
                if (f.exists())
                {
                    f.renameTo(new File(HARRIS_BASE + File.separator + ERROR + File.separator + file));
                }

            }
        }
    }

Answer 1

As a direct answer to the question in your title - nowhere. There is nowhere in this code that needs a Thread.interrupt().

The fact that the thread name is Thread-1708 does not necessarily mean there are 1708 threads. One can choose arbitrary names for threads. I usually include the name of the executor or service in the thread name. Maybe 1600 are now long stopped and there are only around a hundred alive. Maybe this particular class starts naming at 1700 to distinguish from other uses.

1708 threads may not be a problem. If you have a multi-threaded server that is serving 2000 connections in parallel, then it certainly expectable that there are 2000 threads doing that, along with a bunch of other threads.

You have to understand why the sleep is there and what purpose it serves. It's not there to just hog memory for nothing.

Translating the code to "plaintext" (btw it can be greatly simplified by using try-with-resources to acquire and close the connection):

• Acquire a connection
• Use the connection to send (I guess) whatever is in the stack
• When failed or finished - wait 80ms (THIS is your sleep)
• If run flag is still set - repeat from step 1
• Finish the thread.

Now reading through this, it's obvious that it's not the sleep that's the problem. It's that the run flag is not set to false. And your thread just continues looping, even if it can't get the connection at all - it will simply spend most of its time waiting for the retry. In fact - even if you completely strip the sleep out (instead of interrupting it mid-way), all you will achieve is that the Threads will start using up more resources. Given that you have both a logger and you print to stdout via printStackTrace, I would say that you have 2 problems:

• Something is spawning threads and not stopping them afterwards (not setting their run flag to false when done)
• You are likely getting exceptions when getting the Connection, but you never see them in the log.

It might be that the Thread is supposed to set it's own run flag (say when the stack is drained), but you would have to decide that yourself - that depends on a lot of specifics.

Answer 2

Not an answer but some things you should know if you are writing code for a live, production systemn:

:-( Variable and method both have the same name, run. A better name for the variable might be keep_running Or, change the sense of it so that you can write while (! time_to_shut_down) { ... }

:-( Thread.sleep(80) What is this for? It looks like a big red flag to me. You can never fix a concurrency bug by adding a sleep() call to your code. All you can do is make the bug less likely to happen in testing. That means, when the bug finally does bite, it will bite you in the production system.

:-( Your run() method is way too complicated (the keyword try appears four times). Break it up, please.

:-( Ignoring five different exceptions catch (MumbleFoobarException e) { e.printStackTrace(); } Most of those exceptions (but maybe not the InterruptedException) mean that something is wrong. Your program should do something more than just write a message to the standard output.

:-( Writing error messages to standard output. You should be calling log.error(...) so that your application can be configured to send the messages to someplace where somebody might actually see them.