5

Suppose I declare like this:

 private static  Boolean isCondition = false;

Then I am using this like below in synchronized statement:

synchronized(isCondition){
               isCondition = true;
               somestuff();
             }

Here my question is if I update isCondition then it will get a new reference due to autoboxing and if new thread will come in synchronized block then they will get lock on new object enter into synchronized block. This I dont want to happen.

So please suggest me alternatives and if I use volatile then how exactly it will prevent this like below:

private static volatile Boolean isCondition = false;

The actual code is like that:

package com.test.spring.utils;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.springframework.beans.BeansException;
import org.springframework.context.ApplicationContext;
import org.springframework.context.ApplicationContextAware;
import org.springframework.context.support.ClassPathXmlApplicationContext;

 /**
  * @author Pratik
*/
public class TouchPointsSpringContext implements ApplicationContextAware
 {
 private static final Log g_log = LogFactory.getLog(TouchPointsSpringContext.class);
 private static ApplicationContext CONTEXT;
 private static volatile Boolean isServiceInitialized = false;

/**
 * This method is called from within the ApplicationContext once it is done
 * starting up, it will stick a reference to itself into this bean.
 *  
 * @param context
 *            a reference to the ApplicationContext.
 */
public void setApplicationContext(ApplicationContext context) throws BeansException
{
    CONTEXT = context;
}

private static void initializeTouchPointService()
{
    g_log.info("getting touchpoints service application context");
    String[] locations =
            { "appContext-main.xml", "appContext-hibernate.xml" };
    ApplicationContext applicationContext = new ClassPathXmlApplicationContext(locations);
    g_log.info("setting touchpoints service application context");
    CONTEXT = applicationContext;
}
 /**
 * This is about the same as context.getBean("beanName"), except it has its
 * own static handle to the Spring context, so calling this method
 * statically will give access to the beans by name in the Spring
 * application context. As in the context.getBean("beanName") call, the
 * caller must cast to the appropriate target class. If the bean does not
 * exist, then a Runtime error will be thrown.
 * 
 * @param beanName
 *            the name of the bean to get.
 * @return an Object reference to the named bean.
 */
public static Object getBean(String beanName)
{
    if (!isServiceInitialized || (CONTEXT == null))
    {
        synchronized (isServiceInitialized)
        {
            if (!isServiceInitialized)
            {
                initializeTouchPointService();
                isServiceInitialized = true;
            }
        }
    }
    return CONTEXT.getBean(beanName);
}

public static void main(String[] args)
{
    TouchPointsSpringContext.getBean("lookupService");
}

}

4 Answers4

6

Using a Boolean as a lock is a very bad idea: you are effectively using a global variable Boolean.TRUE/FALSE which any other parts of your code can access and potentially deadlock your code.

And using a non final variable as a lock is an even worse idea: everytime you reallocate the instance (isCondition = true) you change your lock, meaning that two threads may execute your synchronized block concurrently, which kind of defeats the whole idea.

So I would recommend a standard idiom:

private static final Object lock = new Object();
private static boolean isCondition;

synchronised(lock) {
    isCondition = true;
    // ...
}
assylias
  • 321,522
  • 82
  • 660
  • 783
  • Thanks for reply Assylias but here i am talking about volatile boolean added sample code. Can you look once and suggest how volatile will help in this case. – Pratik Suman Jul 02 '14 at 14:34
  • 1
    This answer is only partially correct due to use of double-checked-locking in example code. – lexicalscope Jul 02 '14 at 15:03
  • @flamingpenguin not sure I understand your comment - the answer is applicable to the additional code that the OP posted. – assylias Jul 02 '14 at 17:01
  • 1
    The question is about when to use **volatile**, and the OP's code example contains double-checked-locking, for which the correct use of volatile is crucial. Although what you have written does correctly identify and fix a bug in the OP's code, I do no think it actually addresses the OP's problem (which was not clear until after the edit to the question), which I think is related to lazy initialisation and volatile. – lexicalscope Jul 02 '14 at 17:16
3

I think most of the other answers here are not completely right. It is a little bit difficult to understand what you are doing because you do not include the code for initializeTouchPointService, however you appear to be doing something which is a variation on the "Double Checked Locking" idiom.

It is difficult to get this concurrency idiom right and if you are using a version of Java before 5, then you should not attempt to use this idiom at all. I will assume you are using Java 5+.

The important parts of your code are:

private static ApplicationContext CONTEXT;
private static volatile Boolean isServiceInitialized = false;

...

if (!isServiceInitialized || (CONTEXT == null))
{
    synchronized (isServiceInitialized)
    {
        if (!isServiceInitialized)
        {
            initializeTouchPointService();
            isServiceInitialized = true;
        }
    }
}

Assuming that you are using Java 5 or above, you must use volatile on all relevant variables to get this idiom to work correctly. You must also re-check the full condition inside the synchronized block.

You must not use a Boolean as your lock, since Boolean objects are immutable you will get a different object when you change the condition from false to true. Instead use a separate lock object and boolean primitive for the condition.

    private final Object lock = new Object();
    private volatile boolean isServiceInitialized;
    private volatile ApplicationContext context; 

    public Object getBean(String beanName) {
        if (!isServiceInitialized || context == null) {
            synchronized(lock) {
                if (!isServiceInitialized || context == null) {
                    initializeTouchPointService();
                    isServiceInitialized = true;
               }
            }
        }
        return CONTEXT.getBean(beanName);
    }

However, locks in recent versions of Java have very good performance on most architectures. So using the double-checked locking idiom may not make your program much faster - especially compared to how slow spring reflection will be when calling getBean.

Instead of your double-checked design, how about the following simpler design which also avoids volatile:

   private final Object lock = new Object();
   private boolean isServiceInitialized;
   private ApplicationContext context; 

   private ApplicationContext context() {
       synchronized(lock) {
            if (!isServiceInitialized || context == null) {
                initializeTouchPointService();
                condition = true;
            }
            return context;
        }
   }

   public Object getBean(String beanName) {
        return context().getBean(beanName);
   }

I also recommend avoiding the use of static where ever possible, as writing unit tests in the presence of global variables can be tricky. I would seriously consider if there is any way you can change your design to reduce or eliminate your use of static state.

============ edit

Based on my best guess of what the OP is trying to achieve, perhaps this would be better. However, it removes the lazy initialisation. So if you program sometimes refers to this TouchPointsSpringContext class without using the getBean() method then you don't want this answer.

public class TouchPointsSpringContext
{
  private static final Log g_log = LogFactory.getLog(TouchPointsSpringContext.class);
  private static ApplicationContext CONTEXT = initializeTouchPointService();

  private static ApplicationContext initializeTouchPointService()
  {
      g_log.info("getting touchpoints service application context");
      String[] locations =
        { "appContext-main.xml", "appContext-hibernate.xml" };
      ApplicationContext applicationContext = new ClassPathXmlApplicationContext(locations);
      g_log.info("setting touchpoints service application context");
      return applicationContext;
  }

  public static Object getBean(String beanName) 
  {
      return CONTEXT.getBean(beanName);
  }

  public static void main(String[] args)
  {
      TouchPointsSpringContext.getBean("lookupService");
  }
}

Note that the JVM will automatically make sure that your static CONTEXT gets initalised exactly once.

Or alternatively, if you can avoid implementing "ApplicationContextAware" (implementing it seems unnecessary given the rest of the code), but you need to keep he lazy initialisation, then this might be better:

public class TouchPointsSpringContext
{
  private static final Log g_log = LogFactory.getLog(TouchPointsSpringContext.class);
  private static volatile ApplicationContext CONTEXT;
  private static final Object lock = new Object();

  private static ApplicationContext initializeTouchPointService()
  {
    g_log.info("getting touchpoints service application context");
    String[] locations =
        { "appContext-main.xml", "appContext-hibernate.xml" };
    ApplicationContext applicationContext = new ClassPathXmlApplicationContext(locations);
    g_log.info("setting touchpoints service application context");
    return applicationContext;
  }

  public static Object getBean(String beanName)
  {
    if (CONTEXT == null)
    {
        synchronized (lock)
        {
           if (CONTEXT == null)
           {
              CONTEXT = initializeTouchPointService();
           }
        }
    }
    return CONTEXT.getBean(beanName);
   }

   public static void main(String[] args)
   {
       TouchPointsSpringContext.getBean("lookupService");
   }
}
lexicalscope
  • 7,158
  • 6
  • 37
  • 57
  • Thanks for reply. I messed up method name whole editing. Actually initializeService should be initializeTouchPointService. Edited in code also. Sorry for that. – Pratik Suman Jul 02 '14 at 15:26
  • Please review my code once again and suggest anything else then your previous answer. – Pratik Suman Jul 02 '14 at 15:31
  • @flamingpenguin great answer! I just wonder whether `CONTEXT` needs to be volatile in your first code block, since there is a happens-before relation between write to `context` and write to `isServiceInitialized` (program order) and between write to `isServiceInitialized` and any subsequent read of it (volatile) which must precede the read of `context` (program order). Therefore I believe `context` need not be volatile. Am I right? – ciamej Jul 02 '14 at 15:40
  • I can't see the write to context, I assuming the write to context is inside initializeTouchPointService? If it is then I'm not sure why you need to check both context==null and !isServiceInitialized. You could remove "isServiceInitialized" completely and use a single volatile "CONTEXT" variable, and context==null check as your condition. – lexicalscope Jul 02 '14 at 16:01
  • Yes, it's there in the OP's code. Good question - why check `CONTEXT == null`? I think the OP needs to rethink that. – ciamej Jul 02 '14 at 16:22
  • Ahh, I think the questions was edited. I think it depends on when the "setApplicationContext" method can get called. But either the CONTEXT == null or the isServiceInitialized is unnecessary, depending on exactly under what circumstance CONTEXT can be set. – lexicalscope Jul 02 '14 at 16:25
1

Not a full answer, but: Several people here said things like "You must not use Boolean as a lock because..."

Those explanations complicate what should be a simple idea. When you write synchronized (foo) { ... }, you are not synchronizing on the variable foo, you are synchronizing on some object that is the result of the expression, foo.

You did something like this in your example:

Boolean isCondition = ...;

synchronized(isCondition) {
    isCondition = true;
    ...
}

When a thread enters that synchronized block, it acquires the monitor for a particular instance of the Boolean class. Then, the next thing that it does is assign isCondition. The same variable now points to a different instance.

When a second thread tries to enter the same block, it will attempt to synchronize on the new instance, and it will succeed even if the first thread still is in the block. The only thing that synchronized prevents is, it prevents two different threads from synchronizing on the same instance at the same time. In your example, the two different threads synchronized on two different instances, and that is allowed.

Never do this:

synchronized ( foo ) {
    ...
    foo = ...;
    ...
}

A good practice is, if you are going to put a simple variable name in the parens (and that's by far the most common use-case), then make it a final variable.

final MyThingummie myThingummie = new MyThingummie(...);

synchronized ( myThingummie ) {
    ...
}
Solomon Slow
  • 25,130
  • 5
  • 37
  • 57
  • Thanks for reply James but same thing i have explained in my question also. The doubt was how volatile Boolean act in this situation. I mean kind of effect by volatile on same variable. Whether it does any help, if yes then how or if not then why. – Pratik Suman Jul 03 '14 at 14:24
  • @PratikSuman, Look at my example where I said, "Never, ever do this". Assigning the variable foo in that example either is a mistake, or else your code is too insidious to live. Assuming it's a mistake, then making foo "volatile" won't fix it. In fact, adding "volatile" makes it certain that the second thread will see the new value of foo, and therefore be able to enter the synchronized block before the first thread has left it. If you write "synchronized(foo)..." then foo should be _final_, not volatile. – Solomon Slow Jul 03 '14 at 14:39
0

As some others have suggested in comments, you could synchronize on something else and avoid this problem. Define a new variable to lock on:

private final Object lock;

Now change your code a bit:

synchronized(lock) {
           isCondition = true;
           somestuff();
}

You can also achieve similar functionality without the variable by having all of this in a synchronized method.

Solomon Slow
  • 25,130
  • 5
  • 37
  • 57
EJDinki
  • 21
  • 1