Throw exception vs synchronized

I have a method that many threads access in parallel which uses a class with two synchronized methods that I have no control over. getObject and createNewObject. I want to be sure that I do not create several objects (MyObject).

MyObject obj;
public void method1() {
   obj = getObject("key");
   if (obj == null)
      obj = createNewObject("key");
  }

this, I think, would not work as the thread could be suspended between the getting and creating methods so another thread could come in and create an object as well. The synchronized createNewObject method fixes this by checking if an object already exists named "key" and throwing an exception in that case.

Which of the following methods would be preffered? Performance, safety and design wise. I've heard that the double locking type (method 3) doesn't work? Maybe I should just use the method1?

Most of the times, the object will be found so there's no problem. It might be bette开发者_JAVA百科r performance to skip synchronization and handle the exception in those rare cases?

MyObject obj;
public synchronized void method1() {
   obj = getObject("key");
   if (obj == null)
      obj = createNewObject("key");
  }

public void method2() {
   obj = getObject("key");
   if (obj == null)
       try {
          obj = createNewObject("key");
       } catch (Exception e) { // ops, someone already created object "key"
            obj = getObject();
       }
  }

public void method3() {
   obj = getObject("key");
   if (obj == null)
       obj = getObj("key");
}
public synchronized MyObject getObj(String key) {
    MyObject obj = getObject(key);
    if (obj == null)
        obj = createNewObject(key);
    return obj;
 }

---

Start off using method1 until a profiler tells you it is a bottle-neck. It is the cleanest implementation and you know it will work correctly all of the time. If later you see data that shows that you are wasting lots of time with successive calls, then you can think about trying something else.

---

This needs some testing and profiling, but I'm fairly sure you won't gain any significant performance by using any tricks because the synchronization will be performed in any case, as you call getObject() method every time, which is synchronized. So this is not "synchronization / no synchronization" kind of difference, but rather "synchronization / double synchronization" which shouldn't be that much. If you are synchronizing anyway, it is better to do it to the full extent. Which means method1() in your case.

UPDATE

While method2() may look promising too, I have just realized a problem with it: since it doesn't synchronize the write to the obj field, other threads may not see its updated value. So if the obj fields is accessed by other threads than the thread that calls method2(), then method2() isn't correct.

If you make the obj field volatile, I believe it may work (not 100% sure though) since getObject() is synchronized so there should be no "volatile reference to a non-volatile object" problem. After getObject() returns, it performs a write barrier, so it is guaranteed that a fully initialized object will exist in the main memory. And since no thread has a locally cached copy of that object, it should be okay for any thread to access the obj field. Unless the object referenced by the obj field is mutable, in which case all access to it should be synchronized anyway.

This still doesn't make much sense, though. A completely non-synchronized read access is still impossible, so a clean implementation is still better than a "smart" one.

---

Synchronisation in modern VMs consumes very little in resources/time of execution. I would simply synchronise around the check/create methods. Premature optimisation will cost you a lot of time/heartache and you're better off worrying about this sort of thing if and when it becomes a problem.

---

You say you have no control over createNewObject, so under those circumstances method1 is the right answer and I just upvoted someone who said so. But it sounds to me like createNewObject is poorly designed. If it's going to check if the object already exists, then in that case it should return that object rather than throwing an exception. It's silly to require a caller to check if the object exists, and if it doesn't call a function that then repeats the check of whether the object exists.

---

EDIT: Below I wrote the Double checked locking ideom, but it has some significant flaws, which are described in

• Java theory and practice: Fixing the Java Memory Model, Part 1 and
• Java theory and practice: Fixing the Java Memory Model, Part 2

---- Original answer below ----

The best solution is this:

Object obj;

public Object getObject() {
    if( obj == null ) {
        synchronized(this) { // or on something else
            if( obj == null ) {
                obj = createObject();
            }
        }
    }
    return obj;  
}   

private Object createObject() {
    ...
} 

It has the advantage that synchronizing occures only in the critical creation phase of the object, but still works.

---

After reading a bit, I believe a really best answer would be to use the Initialize-On-Demand Holder Class idiom:

private static class LazySomethingHolder {
    public static Something something = new Something();
}

public static Something getInstance() {
    return LazySomethingHolder.something;
}

It has no concurrency problems and no locking even for volatile variables on the common path.