Should you synchronize the run method? Why or why not?

Question

I have always thought that synchronizing the run method in a java class which implements Runnable is redundant. I am trying to figure out why people do this:

public class ThreadedClass implements Runnable{
    //other stuff
    public synchronized void run(){
        while(true)
             //do some stuff in a thread
        }
    }
}

It seems redundant and unnecessary since they are obtaining the object's lock for another thread. Or rather, they are making explicit that only one thread has access to the run() method. But since its the run method, isn't it itself its own thread? Therefore, only it can access itself and it doesn't need a separate locking mechanism?

I found a suggestion online that by synchronizing the run method you could potentially create a de-facto thread queue for instance by doing this:

 public void createThreadQueue(){
    ThreadedClass a = new ThreadedClass();
    new Thread(a, "First one").start();
    new Thread(a, "Second one, waiting on the first one").start();
    new Thread(a, "Third one, waiting on the other two...").start();
 }

I would never do that personally, but it lends to the question of why anyone would synchronize the run method. Any ideas why or why not one should synchronize the run method?

Twentyone

the queue is faulty (the Object monitor is not fair, and the second thread might get to run before the first), the only reason I can imagine is to ensure that when a runnable is submitted twice to a executor/thread it doesn't create races – Wotton

@irreputable My professor did it in an example. I would never personally--except I am waiting to see if there is any brilliant reason for doing it that no one hasn't been pointed out yet. – Twentyone

@ratchet Good point. I guess you would only want the synchronized run it if there is a strange reason why another Thread might be executed on the same object. But even then, I would solve it differently I think. – Twentyone

@Twentyone an atomic boolean hasRun and a if(!hasRun.CompareAndSwap(false,true))return; in the run is better (as it doesn't block a thread and ensures the run is only executed once) but requires extra code and a separate var – Wotton

It's quite odd. I'm a bit worried about the teaching you are getting. You obviously can see through the fog, but that's probably not the case for everyone in your class. It's an annoying situation when you have to do homework: you do the normal thing or the thing your professor does? – Superlative

Answer 1

Synchronizing the run() method of a Runnable is completely pointless unless you want to share the Runnable among multiple threads and you want to sequentialize the execution of those threads. Which is basically a contradiction in terms.

There is in theory another much more complicated scenario in which you might want to synchronize the run() method, which again involves sharing the Runnable among multiple threads but also makes use of wait() and notify(). I've never encountered it in 21+ years of Java.

Thee

@KenyakornKetsombut Wrong how exactly? You have just encountered what other example? Evidence please, not just anecdote. This is science, among other things. – Thee

What if the Runnable has to read a shared object? I just had a NPE in a block with if (sharedObject != null) { threadObject = sharedObject.getField(); } because sharedObject was nullified by another thread between the two instructions. Would it be stupid to declare my run method synchronized? This wouldn't be in contradiction of the purpose (running in a background thread), it just would block writes to the shared object when the thread is running. – Jadwiga

@BenoitDuffez Then the code should synchronize on the shared object while it is being accessed. Not on the Runnable for its entire duration. – Thee

@Thee While this a valid remark, I would add that "the code should synchronize on special-purpose lock object - a private field declared next to run() method, for example", as the shared object may be nullified by another process or task as @BenoitDuffez pointed out, and you cannot use synchronized() on something that may be null. – Menology

Answer 2

There is 1 advantage to using synchronized void blah() over void blah() { synchronized(this) { and that is your resulting bytecode will be 1 byte shorter, since the synchronization will be part of the method signature instead of an operation by itself. This may influence the chance to inline the method by the JIT compiler. Other than that there is no difference.

The best option is to use an internal private final Object lock = new Object() to prevent someone from potentially locking your monitor. It achieves the same result without the downside of the evil outside locking. You do have that extra byte, but it rarely makes a difference.

So I would say no, don't use the synchronized keyword in the signature. Instead, use something like

public class ThreadedClass implements Runnable{
    private final Object lock = new Object();

    public void run(){
        synchronized(lock) {
            while(true)
                 //do some stuff in a thread
            }
        }
    }
}

Edit in response to comment:

Consider what synchronization does: it prevents other threads from entering the same code block. So imagine you have a class like the one below. Let's say the current size is 10. Someone tries to perform an add and it forces a resize of the backing array. While they're in the middle of resizing the array, someone calls a makeExactSize(5) on a different thread. Now all of a sudden you're trying to access data[6] and it bombs out on you. Synchronization is supposed to prevent that from happening. In multithreaded programs you simply NEED synchronization.

class Stack {
    int[] data = new int[10];
    int pos = 0;

    void add(int inc) {
        if(pos == data.length) {
            int[] tmp = new int[pos*2];
            for(int i = 0; i < pos; i++) tmp[i] = data[i];
            data = tmp;
        }
        data[pos++] = inc;
    }

    int remove() {
        return data[pos--];
    }

    void makeExactSize(int size) {
        int[] tmp = new int[size];
        for(int i = 0; i < size; i++) tmp[i] = data[i];
        data = tmp;
    }
}

Utopianism

I like the enthusiasm with the byte code reference, but I really mean is there any benefit to synchronizing the method at all. My professor always writes "synchronized void run()" and I have never done that nor needed to. But that byte code bit is interesting-- – Twentyone

The answer to that is longer than a comment. I will edit it in the answer. – Utopianism

actually the synchronized in the signiture means that the JIT/JVM can keep holding the lock when calling several synchronized methods (i.e. the JVM doesn't release the lock (and immediately reacquires it) when the next operation is calling the next synchronized method) – Wotton

Ok that's true - reacquiring an already held lock should be cheaper - but then I somehow doubt that's noticeable in most situations and I've yet to see a situation where I'd wanted a synchronized method on a instance of the thread (you usually have higher order data structures for communication imo) – Nimitz

@voo I meant that calling t.doThis();t.doThat(); when they are both declared synchronized allows the JVM to keep holding the lock between calling this and that – Wotton

@ratchet I was under the impression the JIT was capable that optimization even for locks declared in the method, and for locks privately held (as opposed to the calling object's monitor). I could be mistaken as I'm unable to immediately produce a source for it. – Utopianism

@ratchet freak Yeah I understood you. But at least if the methods are inlined this could be done anyhow and then I've never stumbled upon a situation where I wanted to make the thread synchronize on its own instance. Usually you want several threads to synchronize on some higher level structure (ie in glowcoder's example the stack would hardly implement run(), but some threads would operate on it and those would need the synchronization). Maybe I'm missing some usual scenario, but I can't imagine one. – Nimitz

same benefits with lock as in the answer are described here: docs.oracle.com/javase/tutorial/essential/concurrency/… – Inapproachable

Answer 3

Why? Minimal extra safety and I don't see any plausible scenario where it would make a difference.

Why not? It's not standard. If you are coding as part of a team, when some other member sees your synchronized run he'll probably waste 30 minutes trying to figure out what is so special either with your run or with the framework you are using to run the Runnable's.

Superlative

Answer 4

From my experience, it's not useful to add "synchronized" keyword to run() method. If we need synchronize multiple threads, or we need a thread-safe queue, we can use more appropriate components, such as ConcurrentLinkedQueue.

Brendanbrenden

Answer 5

Well you could theoretically call the run method itself without problem (after all it is public). But that doesn't mean one should do it. So basically there's no reason to do this, apart from adding negligible overhead to the thread calling run(). Well except if you use the instance multiple times calling new Thread - although I'm a) not sure that's legal with the threading API and b) seems completely useless.

Also your createThreadQueue doesn't work. synchronized on a non-static method synchronizes on the instance object (ie this), so all three threads will run in parallel.

Nimitz

"...so all three threads will run in sequence." Thats sort of what I meant be a "de-facto" thread queue... they would just run after the prior one finished. Or did I misunderstand you? (And like I said, if i needed something like that I would never code it like that... this is just speculation.) – Twentyone

No I just got confused. This should say "run in parallel" or something like that. I'll fix it. Basically if the run method contained only a print() statement you could every possible combination of the three sentences. – Nimitz

Uh actually misread the example, so yes that'd work - somewhat strange construct though. You could get exactly the same result with just a simple for(int i = 0; i < N; i++) a.run(); without the overhead. – Nimitz

Answer 6

Go through the code comments and uncomment and run the different blocks to clearly see the difference, note synchronization will have a difference only if the same runnable instance is used, if each thread started gets a new runnable it won't make any difference.

class Kat{

public static void main(String... args){
  Thread t1;
  // MyUsualRunnable is usual stuff, only this will allow concurrency
  MyUsualRunnable m0 = new MyUsualRunnable();
  for(int i = 0; i < 5; i++){
  t1 = new Thread(m0);//*imp*  here all threads created are passed the same runnable instance
  t1.start();
  }

  // run() method is synchronized , concurrency killed
  // uncomment below block and run to see the difference

  MySynchRunnable1 m1 = new MySynchRunnable1();
  for(int i = 0; i < 5; i++){
  t1 = new Thread(m1);//*imp*  here all threads created are passed the same runnable instance, m1
  // if new insances of runnable above were created for each loop then synchronizing will have no effect

  t1.start();
}

  // run() method has synchronized block which lock on runnable instance , concurrency killed
  // uncomment below block and run to see the difference
  /*
  MySynchRunnable2 m2 = new MySynchRunnable2();
  for(int i = 0; i < 5; i++){
  // if new insances of runnable above were created for each loop then synchronizing will have no effect
  t1 = new Thread(m2);//*imp*  here all threads created are passed the same runnable instance, m2
  t1.start();
}*/

}
}

class MyUsualRunnable implements Runnable{
  @Override
  public void  run(){
    try {Thread.sleep(1000);} catch (InterruptedException e) {}
}
}

class MySynchRunnable1 implements Runnable{
  // this is implicit synchronization
  //on the runnable instance as the run()
  // method is synchronized
  @Override
  public synchronized void  run(){
    try {Thread.sleep(1000);} catch (InterruptedException e) {}
}
}

class MySynchRunnable2 implements Runnable{
  // this is explicit synchronization
  //on the runnable instance
  //inside the synchronized block
  // MySynchRunnable2 is totally equivalent to MySynchRunnable1
  // usually we never synchronize on this or synchronize the run() method
  @Override
  public void  run(){
    synchronized(this){
    try {Thread.sleep(1000);} catch (InterruptedException e) {}
  }
}
}

Quito