10

I'm using a ThreadPoolExecutor in Java to manage a lot of running threads. I've created my own simple ThreadFactory so I can give the threads better names.

The issue is that the name gets set in the Thread when the thread pool is first created and is not tied to the task that the thread pool is actually running. I understand this... my Runnables and Callables--though they have names--are actually one level of abstraction down from the ThreadPoolExecutor's running threads.

There have been some other questions on StackOverflow about creating names for ThreadPoolExecutor thread pools. (See How to give name to a callable Thread? and How to name the threads of a thread pool in Java.)

What I want to know is: does anyone have a good solution for keeping the name of the thread pool thread in sync with the Runnable that it is actually running?

i.e. If I call Thread.getCurrentThread().getName() I'd like it not to return the name of the top-level thread pool, but rather the name of the Callable/Runnable that the thread is currently running.

Since this is mainly for debugging and logging purposes, I'm trying to avoid a solution that involves me putting new code into every Runnable that might be submitted to the ThreadPoolExecutor--I'd rather just put some code into the ThreadFactory or wrap the ThreadPoolExecutor itself so that the change is done in one place. If such a solution doesn't exist I probably won't bother since it's not mission critical.

begin edit To clarify, I know I can put a Thread.currentThread().setName( "my runnable name" ); as the first line of every Runnable's run method, but I'm trying to avoid doing that. I'm being a perfectionist here, and I realize it, so I won't be offended if people want to comment on this question and tell me so. end edit

My other question, I suppose, is whether people think it's a bad idea to do such a thing. Should I be wary of updating the thread pool name like this?

Thanks for any suggestions!

Community
  • 1
  • 1
Jeff Goldberg
  • 931
  • 1
  • 9
  • 20
  • "keeping the name of the thread pool thread in sync with the Runnable that it is actually running" --> Runnable / Callable does not define a name, so I can't quite see the thrust of your question. Does your ThreadFactory actually give each thread a distinct name? – serg10 Dec 15 '11 at 16:41
  • Right, each of my Runnables and Callables implement a Named interface, so I do have access to names. I realize that I'm being picky in that I've already implemented a special feature for all my Runnables but want to avoid adding additional code that manually sets the thread name. – Jeff Goldberg Dec 15 '11 at 16:51
  • 1
    Edited for clarity, changing some "thread pool" to "thread", and some "thread" to "task". – Andy Thomas Dec 15 '11 at 17:01

3 Answers3

15

Create a ThreadPoolExecutor that overrides the beforeExecute method.

private final ThreadPoolExecutor executor = new ThreadPoolExecutor (new ThreadPoolExecutor(10, 10,  0L, TimeUnit.MILLISECONDS,  new LinkedBlockingQueue<Runnable>()){   
    protected void beforeExecute(Thread t, Runnable r) { 
         t.setName(deriveRunnableName(r));
    }

    protected void afterExecute(Runnable r, Throwable t) { 
         Thread.currentThread().setName("");
    } 

    protected <V> RunnableFuture<V> newTaskFor(final Runnable runnable, V v) {
         return new FutureTask<V>(runnable, v) {
             public String toString() {
                return runnable.toString();
             }
         };
     };
}

Not sure how exactly derveRunnableName() would work, maybe toString()?

Edit: The Thread.currentThread() is in fact the thread being set in beforeExecute which calls the afterExecute. You can reference Thread.currentThread() and then set the name in the afterExecute. This is noted in the javadocs

/**
 * Method invoked upon completion of execution of the given Runnable.
 * This method is invoked by the thread that executed the task. If
 * non-null, the Throwable is the uncaught <tt>RuntimeException</tt>
 * or <tt>Error</tt> that caused execution to terminate abruptly.
 *
 * <p><b>Note:</b> When actions are enclosed in tasks (such as
 * {@link FutureTask}) either explicitly or via methods such as
 * <tt>submit</tt>, these task objects catch and maintain
 * computational exceptions, and so they do not cause abrupt
 * termination, and the internal exceptions are <em>not</em>
 * passed to this method.
 *
 * <p>This implementation does nothing, but may be customized in
 * subclasses. Note: To properly nest multiple overridings, subclasses
 * should generally invoke <tt>super.afterExecute</tt> at the
 * beginning of this method.
 *
 * @param r the runnable that has completed.
 * @param t the exception that caused termination, or null if
 * execution completed normally.
 */
protected void afterExecute(Runnable r, Throwable t) { }

Edit The TPE will wrap the Runnable within a FutureTask, so to support the toString method you could override newTaskFor and create your own wrapped FutureTask.

John Vint
  • 39,695
  • 7
  • 78
  • 108
  • I'm actually trying something like this right now, so good suggestion. The only issue is when the Runnable is complete, the thread pool still has that name. Unfortunately the overridable afterExecute method doesn't get the Thread in which the Runnable was run. I'd like to be able to clean it up when I'm done too. – Jeff Goldberg Dec 15 '11 at 17:04
  • 1
    @JeffGoldberg In afterExecute, the Thread.currentThread() is acutally the thread I am setting the name of in beforeExecute. I will update my answer to demonstrate this. – John Vint Dec 15 '11 at 17:57
  • I made one adjustment, and that was to add a `Map originalThreadNameMap` so that I could reset the thread back to its original name in the `afterExecution` method, but that's tied to how my specific program is naming threads. Anyway, it works great and I like it more than overriding the execute method and it seems cleaner than wrapping in a new Runnable. Thank you for your help and thank you for reading the JavaDoc more closely than I did. – Jeff Goldberg Dec 15 '11 at 18:44
  • 1
    No Problem! Make sure you are using a `ConcurrentMap` :) Otherwise you can run into an infinite loop! http://mailinator.blogspot.com/2009/06/beautiful-race-condition.html – John Vint Dec 15 '11 at 18:57
  • I've run into enough race conditions in this system that I'd already put it into a SynchronizedMap. Since I'm only doing puts and removes, there's no advantage of having the quick reads of ConcurrentMap. Also, I love that article you linked... like its author, I too am always strangely excited when I discover a race condition. – Jeff Goldberg Dec 15 '11 at 19:35
  • In the answer there is a question: "Not sure how exactly deriveRunnableName() would work, maybe toString()?". toString() does not seem to work and as the Runnable in beforeExecute() is not the same as the Runnable I passed to submit() I am not sure how to get to the original runnables name. – Zitrax Jul 11 '13 at 11:52
  • @Zitrax The `deriveRunnableName` method is really specific to the implementation of the runnable. For instance, if in your Runnable, you override the `toString` method you would get the name you expected – John Vint Jul 11 '13 at 11:59
  • @JohnVint I did override toString(), but when I debug I can see that the Runnable that I submit is not what I see in `beforeExecute()`. In `beforeExecute()` it prints `java.util.concurrent.ThreadPoolExecutor$Worker@4516ee8c[State = -1, empty queue]` while in `submit()` it do print my `toString()` implementation. So I am assuming the internal implementation is wrapping my runnable in a Worker, from which my original runnable is not exposed. – Zitrax Jul 11 '13 at 12:52
  • 1
    @Zitrax you are right that the TPE will wrap the Runnable in a FutureTask. I will update my answer to support `toString` – John Vint Jul 11 '13 at 13:09
4

So, I've got a solution that manages both to set the name and clean up after the name. Thank you to both Peter Lawrey and John Vint for their suggestions which led me here. Since neither suggestion fully handled my problem I figured I'd post this sample code as a separate answer. I apologize if that's poor etiquette--if so, let me know and I'll adjust.

In the below code I decided to keep the name of the original ThreadPoolExecutor thread and append the Runnable name, then in the finally block remove the Runnable name to clean up, but that can be easily altered.

As John Vint suggests, I'd prefer to override the beforeExecution method and then override the afterExecution method to clean up, but the afterExecution does not have a handle to the thread.

public class RunnableNameThreadPoolExecutor extends ThreadPoolExecutor {

    /* Constructors... */

    @Override
    public void execute(Runnable command) {
        super.execute(new ManageNameRunnable(command));
    }

    private class ManageNameRunnable implements Runnable {
        private final Runnable command;
        ManageNameRunnable( Runnable command ) {
            this.command = command;
        }

        public void run() {
            String originalName = Thread.currentThread().getName();
            try {
                String runnableName = getRunnableName(command);
                Thread.currentThread().setName(originalName+ ": " + runnableName);
                command.run();
            } finally {
                Thread.currentThread().setName(originalName);
            }
        }
    }
}
Jeff Goldberg
  • 931
  • 1
  • 9
  • 20
3

My suggestion would be to try

pool.execute(new Runnable() {
    public void run() {
         Thread.getCurrentThread().setName("My descriptive Runnable");
         // do my descriptive Runnable
    }
});                 

You can also reset the name when you have finished if you like.

Peter Lawrey
  • 525,659
  • 79
  • 751
  • 1,130
  • Are you suggesting I do this every time I make a call to pool.execute or that I override the pool's execute method? – Jeff Goldberg Dec 15 '11 at 16:59
  • Assuming you want to change the name to reflect the what that thread is doing, yes. Note: it will change when the Runnable starts, not when you call invoke. ;) – Peter Lawrey Dec 16 '11 at 08:22