CODEWITHSUNDEEP
javamultithreadingsynchronized
Anurag
Anurag

Reputation: 733

Correct way to Synchronize Methods in Java

I have a sample class:

public class LocksAndSynchronization {

    private Object lock1 = new Object();
    private Object lock2 = new Object();

    static int count;

    public void methodOne() {

        for (int i = 1; i <= 10000; i++) {
            count++;
        }

    }

    public void methodTwo() {

        for (int i = 1; i <= 10000; i++) {
            count++;
        }

    }

    public synchronized void process() {
        methodOne();
        methodTwo();
    }

    public static void main(String[] args) throws InterruptedException {
        final LocksAndSynchronization l = new LocksAndSynchronization();
        Thread t1 = new Thread() {

            @Override
            public void run() {
                l.process();
            }

        };

        Thread t2 = new Thread() {

            @Override
            public void run() {
                l.process();
            }

        };


        t1.start();
        t2.start();

        t1.join();
        t2.join();


        System.out.println("count: " + count);

    }

}

process() actually makes a call to both the methods. So which is a better approach?

  1. Synchronize methodOne and methodTwo or,
  2. Synchronize process() (as done in the code above) ?

Both of the above options would work. But which of them makes more sense?

Upvotes: 1

Views: 88

Answers (6)

Marko Topolnik
Marko Topolnik

Reputation: 200138

Your simplified code doesn't make it easy to discern the essence of your question. For one, count is static so locking at instance level can't work, but that could just be a distractor.

A more important consideration are methodOne and methodTwo, two public methods which apparently require synchronization, but do none on their own. If you choose to synchronize just the process method, that means that you are requiring all the other callers of methodOne and methodTwo to take proper care of synchronization as well, which looks like quite shaky design.

Moreover, if you don't need the entire process to be atomic, and it can decompose into two separate atomic steps, then again it is more natural to have locking follow those semantics. This depends on whether you'll be calling methodOne and methodTwo from aynwhere else in your program.

Upvotes: 2

Adem
Adem

Reputation: 9429

count is static, so it is same for all object. so you need to syncronize process static way. to do that, create a static lock and use it

static Object staticObject = new Object();
public void process() {
   synchronized( staticObject){
      methodOne();
      methodTwo();
   }
}

or you can synronize methodOne and methodTwo

static Object staticObject = new Object();
public void methodOne() {
    synchronized( staticObject){
      for (int i = 1; i <= 10000; i++) {
          count++;
      }
    }
}

public void methodTwo() {
    synchronized( staticObject){
      for (int i = 1; i <= 10000; i++) {
          count++;
      }
    }
}

moreover, using synchronize keyword in function declaration, it means using object itself as lock. so, if you have multiple object, you block is synchronized according to object.

Upvotes: 3

davmac
davmac

Reputation: 20631

As Adem says, the static 'count' field needs a static lock. However there is a bigger interface design concern here. Both methodOne and methodTwo are public meaning that that they can be called from any other class. If they are required to execute in a synchronised context then it's advisable that they both utilise the synchronisation mechanism.

If it were not the case that methodOne and methodTwo were public, my preference would be to perform the syncrhonisation at the higher level (because this is slightly more efficient and flexible), i.e. in the process method, and then to put a comment on the other two methods which specifies that they must be called from a synchronized context.

Upvotes: 0

Ashalynd
Ashalynd

Reputation: 12563

No matter whether you synchronize process or any of the methods separately, you still will not have consistency between method1 and method2 (i.e. that the variable count would not be changed between these calls). The reason is that the variable count is static, so it's not thread-safe even though it's used from within a synchronized method.

Upvotes: 0

Joe Inner
Joe Inner

Reputation: 1470

I would prefer to synchronize methodOne and methodTwo since they may be called from outside the object. They also can be called without being embedded in process. Therefore each method should be implemented in a threadsafe manner on its own.

Hope that helped.

Upvotes: 2

Michal Wilkowski
Michal Wilkowski

Reputation: 747

It is advisable to synchronize as little piece of code as possible, because it generates contentions (other threads waiting). Therefore approach no 1 is preferable.

Upvotes: 1

Related Questions