CODEWITHSUNDEEP
java
Gam
Gam

Reputation: 1272

Should I try to avoid static synchronized method

From what I've understood the following code is not efficient:

class Foo {
    static Resource resource1;
    static Resource resource2;

    static synchronized void methodA() {
       resource1.add("abc");
    }

    static synchronized void methodB() {
       resource2.add("abc");
    }
}

From what I've understood, both methods lock in a single object (the class object Foo.class), so I'm guessing the following is a good optimization?

class Foo {
    static Resource resource1;
    static Resource resource2;

    static void methodA()  {
       synchronized(resource1) {
           resource1.add("abc");
       }
    }

    static void methodB()  {
       synchronized(resource2) {
           resource2.add("123");
       }
    }
}

As long as the two resources do not depend on each other.

When should I consider using static synchronized method?

Upvotes: 6

Views: 254

Answers (4)

Davide Lorenzo MARINO
Davide Lorenzo MARINO

Reputation: 26946

Your optimization is right.

The first code lock on the Foo.class

The second code lock on two different objects: resource1 and resource2.

Visually you can imagine this

First code:

 Thread 1              Thread 2
 ------------------------------

 Foo.methodA()

                        Foo.methodB()   
 // A call to methodB needs to wait for completion of methodA

Second code:

 Thread 1              Thread 2
 ------------------------------

 Foo.methodA()         Foo.methodB()    
 // At the same time in a machine with at least a dual core

You should consider to use a static synchronized method only when you have one resource to synchronize.

Upvotes: 5

mdewit
mdewit

Reputation: 2036

The second method (locking on the objects) is preferable as it gives you more control over when locking should take place. What's more, it prevents an external party to your class from locking your class indefinitely, preventing your own methods from executing.

Consider the following: Imagine some external code contains the following statements:

 synchronized (Foo.class) {
     Thread.sleep(10000);
 }

Now, if you used synchronized on the class methods themselves as in method 1, other classes concurrently trying to call methodA or methodB will be blocked until the sleep completes. If however you used internal locking on internal objects as in method 2, then the other classes will not have to wait.

Because of the above, I would not really recommend method 1.

PS I just noticed that the inner locks in method 2 were not declared as final. This will be a problem if they are reassigned while a method is busy locking on them (the lock will then be on a different instantiation). To prevent this, rather declare them final as follows:

final static Resource resource1 = new Resource(...);
final static Resource resource2 = new Resource(...);

In short, never synchronize on non-final objects.

Upvotes: 0

noktigula
noktigula

Reputation: 445

The optimization is good, but be aware of possible deadlock. In example, sometime you'll decide to access both resources:

class Foo {
    static Resource resource1;
    static Resource resource2;

    static void methodA()  {
       synchronized(resource1) {
           resource1.add("abc");
           synchronized(resource2) {
               resource2.add("abc");
           }
       }
    }

    static void methodB()  {
       synchronized(resource2) {
           resource2.add("123");
           synchronized(resource1) {
               resource1.add("123");
           }
       }
    }
}

This can lead to deadlock:

  1. Thread A try to execute methodA();
  2. Thread B try to execute methodB() at same time;
  3. Thread A get resource1 lock, Thread B get resource2 lock
  4. Thread A try to get resource2 lock, and start waiting for lock to release
  5. Thread B try to get resource1 lock, and start waiting for lock to release
  6. Deadlock

To avoid this, you can make your Resource class to be thread safe:

class Resource {
    private final Object mLock = new Object();
    ...
    public void add(String str) {
        synchronized(mLock) {
            //do stuff
        }
    }
}

Upvotes: 0

jjmontes
jjmontes

Reputation: 26954

Use the static synchronized construct when your class abstracts access to a single critical resource, thus locking on the class is semantically correct.

If your class abstracts access to more than one critical resource, then you have to use finer locking, as in your example.

You can consider the synchronized modifier for methods as syntactic sugar, there is no extra black magic other than locking on the class (or the instance if the method wasn't static).

In your first example, it's questionable why a single class is providing access to two different critical resources if they are totally unrelated. Perhaps you could move the critical sections to the Resource classes themselves.

Upvotes: 7

Related Questions