Help reviewing the following code, is it thread safe?

Question

private static Callback callback;

public Foo()
{
    super(getCallback());
}

private static Callback getCallback()
{
    callback = new Callback();
    return callback;
}

Constructor Foo() can potentially be called from multiple threads. My concern is with the private static field 'callback' and the static method 'getCallback()'.

As can be seen, every time 'getCallback()' is called, it assigns a new value to static field 'callback'.

My guess is that it is not thread safe because the keyword static is always attached to the class not the instance, so that means, the static field 'callback' of a Foo can potentially be overwritten by other thread that is constructing another Foo(). Is this correct?

Please correct me if I am wrong. Thanks!

EDIT: My intention is to keep 'callback' somewhere in the class, so I can reuse it later on. But this is not easy because Foo extends from a class that has constructor mandating 'callback' to be passed on.

Answer 1

Yes, you are correct. It's possible for two instances of Foo to end up with the same CallBack instance when two threads enter the getCallback() method simultaneously and one assigns a new CallBack to the static field while the other has already done this but not yet returned. In this case, the best fix is not to have the static field, since it serves no purpose. Alternatively, make getCallback() synchronized.

But note that it's not true that only the static keyword results in code that is not threadsafe.

Answer 2

Callback will get a new value every single time Foo() is called (even from the same thread). I'm not quite sure what your code should be doing (if you want to initialize the static variable only once (singleton), you should check if it is still null in getCallback() - and what is actionCallback?). For making it thread-safe, use synchronized.

Answer 3

I think you summed it up perfectly yourself, but without more detail about what you are trying to achieve it will be tricky to make suggestions to fix your problem.

One obvious question is, does callback have to be static? Or could you safely make it an instance field without breaking the functionality of your class?

Answer 4

It's not thread-safe. Try these alternatives:

Option 1: here all instances share the same callback

private static final Callback callback = new Callback();

public Foo() {
    super(callback);
}

Option 2: here each instance has its own callback

public Foo() {
    super(new Callback());
}

Note that in both cases, although the constructor is thread-safe, the thread-safety of the entire class depends on the implementation of Callback. If it has mutable state, then you'll have potential problems. If Callback is immutable, then you have thread-safety.

Answer 5

I know it has been answered but the why has not really been detailed.

It two threads are calling the getCallback() method, they could execute the lines as follows:

1. Thread 1 - callback = new Callback();
2. Thread 2 - callback = new Callback();
3. Thread 1 - return actionCallback;
4. Thread 2 - return actionCallback;

In this case, the callback generated at (2.) would be returned in both (3.) and (4.)

The solution would seem to be to ask why callback defined staticly if it is specific to the instance not class.

I hope that helps.

Answer 6

What you are trying to do is called a Singleton pattern, if you do a search you can find out why its generally a good idea to avoid this pattern if you can, however if you need it you can do the following.

private static final Callback CALLBACK= new Callback();

Or if you need a lazy Singleton you can do

public class Foo {
   class CallbackHolder {
       static final Callback CALLBACK= new Callback();
   }

   public static Callback getCallback() {
      return CallbackHolder.CALLBACK;
   }

public Foo() {
    super(getCallback());
}

Both implementations are thread safe.

Answer 7

Do you want one callback per thread, one per object, or a true Singleton?

Some sketches on how to do the different variants - just from the top of my head, don't take these too literally :)

Please note that I've assumed that the Callback has a nontrivial constructor that might throw exception that needs to be handled, if it's a trivial constructor you can simplify all of these a lot.

One per thread:

  private static ThreadLocal<Callback> callback;

  public Foo()
  {
      super(getCallback());
  }

  private static Callback getCallback()
  {
      if ( callback.get() == null ) 
          callback.set(new Callback());
      return callback.get();
  }

Single callback for all threads:

  private final static Callback callback;

  static {
      callback = new Callback(); 
  }

  public Foo()
  {
      super(getCallback());
  }

  private static Callback getCallback()
  {
      return callback;
  }

And, for completness, one callback per object:

  private Callback callback;

  public Foo()
  {
      super(getCallback());
  }

  private Callback getCallback()
  {
      callback = new Callback();
      return callback;
  }