5

I have a class organized as follows:

public class MyClass {

   ExecutorService pool;

   public MyClass(){
     pool = ... //inited by a class that implements ExecutorService
   }

   public final void submit(Runnable run){
        pool.execute(run);
   }
}

Is the method submit thread safe or I should use a Lock-based system? E.g.

   ReentrantLock look  = new ReentrantLock();

   public final void submit(Runnable run){
        lock.lock();
        try{ pool.execute(run); } finally{lock.unlock();}
   }
mat_boy
  • 12,998
  • 22
  • 72
  • 116

2 Answers2

8

No you don't need the lock when calling ExecutorService#submit.

Memory consistency effects: Actions in a thread prior to the submission of a Runnable or Callable task to an ExecutorService happen-before any actions taken by that task, which in turn happen-before the result is retrieved via Future.get().

or when calling Executor#execute:

Memory consistency effects: Actions in a thread prior to submitting a Runnable object to an Executor happen-before its execution begins, perhaps in another thread.

assylias
  • 321,522
  • 82
  • 660
  • 783
2

This depends on how your ExecutorService is accessed or how your MyClass is created. If an instance of MyClass is created in a single thread and the #pool ivar is also created in a single thread then this class is thread-safe and your use of #pool is threadsafe.

However, if your MyClass can exist across multiple threads (such as first accessed through a singleton, shared factory or shared list - i.e. not new MyClass()) then it is not thread-safe. This is because multiple threads could access your Myclass instance and its single #pool ivar. Calling submit could be done but multiple threads at once and so the state of your Myclass instance or pool could be compromised. In this case, your MyClass#submit should be synchronised or managed by locks. As it's a petite method, I'd suggest just synchronising #submit.

Likewise, if your MyClass instance is created in this thread but #pool is not created as new in this thread (pool = new AnExecutorPoolType(...); then this class would again not be threadsafe. The pool may again be accessed from another source(as above) that is available across multiple threads. If that's the case then there is a risk of it being left in an inconsistent state by multiple thread accesses. In this case, the pool#execute method should be synchronised (or through a synchronised block guarding ivar access) or managed through locks.

wmorrison365
  • 5,995
  • 2
  • 27
  • 40
  • Actually, MyClass is an enum designed for use the pool as a singleton. – mat_boy Apr 17 '13 at 12:50
  • I've re-read my answer here with some alarm, thinking 'doesn't look right'. However, I'm going to leave it as the point I think I'm making is... ExecutorService may be threadsafe (though that really depends on the implementation [though java impls are "happens-before"] ), but your implementation of submit may not be thread-safe. It's plausible that a different thread could come in within your `Myclass#submit` and cause the order to be not as you would expect - before reaching the `ExecutorService`. So, your class may need to guard against this. – wmorrison365 Mar 15 '16 at 10:39
  • See also [Is ThreadPoolExecutor thread safe?](http://stackoverflow.com/questions/1702386/is-threadpoolexecutor-thread-safe). – wmorrison365 Mar 15 '16 at 10:40