views:

441

answers:

2

First let's look at the utility class (most javadoc has been removed to simply the example):

public class ApplicationContextUtils {

    /**
     * The application context; care should be taken to ensure that 1) this
     * variable is assigned exactly once (in the
     * {@link #setContext(ApplicationContext)} method, 2) the context is never
     * reassigned to {@code null}, 3) access to the field is thread-safe (no race
     * conditions can occur)
     */
    private static ApplicationContext context = null;

    public static ApplicationContext getContext() {

    if (!isInitialized()) {
        throw new IllegalStateException(
         "Context not initialized yet! (Has the "
          + "ApplicationContextProviderBean definition been configured "
          + "properly and has the web application finished "
          + "loading before you invoked this method?)");
    }

    return context;
    }

    public static boolean isInitialized() {
    return context == null;
    }

    @SuppressWarnings("unchecked")
    public static <T> T getBean(final String name, final Class<T> requiredType) {
    if (requiredType == null) {
        throw new IllegalArgumentException("requiredType is null");
    }
    return (T) getContext().getBean(name, requiredType);
    }

    static synchronized void setContext(final ApplicationContext theContext) {

    if (theContext == null) {
        throw new IllegalArgumentException("theContext is null");
    }

    if (context != null) {
        throw new IllegalStateException(
         "ApplicationContext already initialized: it cannot be done twice!");
    }

    context = theContext;
    }

    private ApplicationContextUtils() {
    throw new AssertionError(); // NON-INSTANTIABLE UTILITY CLASS
    }
}

Finally, there is the following helper Spring managed bean that actually calls the 'setContext' method:

public final class ApplicationContextProviderBean implements
    ApplicationContextAware {

    public void setApplicationContext(
        final ApplicationContext applicationContext) throws BeansException {
    ApplicationContextUtils.setContext(applicationContext);
    }
}

Spring will call the setApplicationContext method once after the app is started. Assuming a nincompoop has not previously called ApplicationContextUtils.setContext(), that should lock in the reference to the context in the utility class, allowing calls to getContext() to success (meaning that isInitialized() returns true).

I just want to know if this class violates any principles of good coding practices, with respect to thread safety in particular (but other stupidities found are welcome).

Thanks for helping me to become a better programmer, StackOverflow!

Regards, LES

P.S. I didn't go into why I need this utility class - let it suffice that I indeed do have a legitimate need to access it from a static context anywhere in the application (after the spring context has loaded, of course).

+7  A: 

No. It's not thread safe.

Writes to the context class variable are not guaranteed to be visible to threads that read that variable through getContext().

At the very least, declare context to be volatile. Ideally, redefine context as an AtomicReference, set through a call like this:

if(!context.compareAndSet(null, theContext))
  throw new IllegalStateException("The context is already set.");


Here's a more complete example:

public class ApplicationContextUtils {

  private static final AtomicReference<ApplicationContext> context = 
    new AtomicReference<ApplicationContext>();

  public static ApplicationContext getContext() {
    ApplicationContext ctx = context.get();
    if (ctx == null)
      throw new IllegalStateException();
    return ctx;
  }

  public static boolean isInitialized() {
    return context.get() == null;
  }

  static void setContext(final ApplicationContext ctx) {
    if (ctx == null) 
      throw new IllegalArgumentException();
    if (!context.compareAndSet(null, ctx))
      throw new IllegalStateException();
  }

  public static <T> T getBean(final String name, final Class<T> type) {
    if (type == null) 
      throw new IllegalArgumentException();
    return type.cast(getContext().getBean(name, type));
  }

  private ApplicationContextUtils() {
    throw new AssertionError();
  }

}

Note that in addition to thread safety, this also provides type safety, taking advantage of the Class instance passed into the getBean() method.

I'm not sure how you plan to use the isInitialized() method; it doesn't seem very useful to me, since as soon as you call it, the condition could change, and you don't have a good way to be notified.

erickson
That's true, but why does it matter in **this** scenario? `setContext()` will only be invoked once, `getContext()` will fail prior (or during) that which is as OP expects.
ChssPly76
`getContext()` can fail **"after"** the call to `setContext()` too. Without a memory barrier, you can't say anything about when one thread sees actions by another thread. In other words, in the absence of *happens-before* relationships as defined in the spec, words like "before", "during", and "after" are meaningless.
erickson
Very well, let me define "during" mentioned in my first comment as "the period up until the point when getter thread sees changes to `context`". And yes, **in theory** this may not _ever_ happen since getter is not synchronized on the same lock and `context` is not volatile. Even in that case, `getContext()` will simply throw an exception - same as it would if `setContext()` was never invoked to begin with. So again, how does any of this **practically** matter in **this** scenario?
ChssPly76
so if a thread was looping, say:while(!ApplicationContextUtils.isInitialized()) { }Service myService = ApplicationContextUtils.getBean("service",Service.class);The JVM could optimize it as:while(true) { ... infinite loop ... }?
LES2
p.s. if i use compareAndSet do I still synchronize the method?
LES2
@LES2: correct, exactly right. The JVM is quite entitled to make such observations if it wishes. It may not ever happen but this is exactly the kind of thing which can stop working when you switch from client to server VM, or from Hotspot to JRockit, or your method gets invoked for the 1,000,001st time which triggers some sort of JIT recompile, or whatever. Should be volatile at the very least.
Cowan
getInitialized() just seemed like a logical method to have. Perhaps even more useful would be a utility method that blocked until the context was loaded. For example, the application code could use:ApplicationContext context = ApplicationContextUtils.getContextWhenLoaded(); // method doesn't return until context has been set (perhaps by using the concurrency utils in an efficient manner)
LES2
the type safety portion is nice, but unnecessary in this instance. the javadoc for the delegate method on the context guarantees that an instance of the type will be returned; the version of spring i'm using isn't generified, so suppressing the warning seemed okay here (note: i always either 1) write type-safe code, or 2) handle type-safety warnings at the most specific level when working with legacy APIs a la Bloch). If the legacy API doesn't document the types it returns, I scream!
LES2
p.s. what's the advantage of using compare and set versus the 1. synchronized setter and 2. a volatile field (assume that i don't need pre-java 5 memory model semantics)
LES2
If you are writing pre-Java 5, you would need to synchronize, because you need an atomic read-write in the `setContext()`. For that reason, I think a `compareAndSet` operation might give equivalent performance to a synchronized method; if that matters, you'd have to profile on your target platform and see. A read-only of a volatile (or AtomicReference) is faster than a `synchronized` read though. What I like about using the `AtomicReference` is the clarity, simplicity, and ease of maintenance of the code. It expresses succinctly but clearly the intent of the coder.
erickson
P.S. Yes, a blocking version of `getContext()` would be much more useful than an `isInitialized()` method that can only be used effectively in a busy wait.
erickson
As far as the design of a blocking getContext() method, i think a good signature would be: public static ApplicationContext getContext(int timeout); As for the implementation, ..., I know enough to know that a naive while(!isInitialized()) { ... sleep / timeout accounting ... } return getContext(); would be stupid (non-performant, but 'correct') ... I'm not sure of the best way to do a blocking algorithm though. I know that Doug Lea's library has features to facilitate this, but I haven't read his book yet (I very seldom need to do concurrent programming)
LES2
A: 

Spring already has a class called ContextSingletonBeanFactoryLocator that wires up static access to the ApplicationContext for you. At the least, using this class might save you the trouble of having to worry about if your custom approach is thread safe.

However it's a little confusing to use this class at first, since there's a little bit of indirection going on. You can take a look at this blog post for more information on how this call works.

Jason Gritman