Sun ServiceLocator pattern implementation incorrect?

Discussions

EJB programming & troubleshooting: Sun ServiceLocator pattern implementation incorrect?

  1. All,

    I posted this in General EJB, but no-one there seems prepared or qualified to comment, maybe you lot will have a go ;-)

    In the ServiceLocator pattern description http://java.sun.com/blueprints/patterns/ServiceLocator.html there is explicit support for multithreaded access by creating the cache map via:

      cache = Collections.synchronizedMap(new HashMap());

    however, when there is a cache miss, there is no synchronization on the InitialContext access, at the method level or anywhere else. The javadoc for InitialContext clearly states:

    "An InitialContext instance is not synchronized against concurrent access by multiple threads. Multiple threads each manipulating a different InitialContext instance need not synchronize. Threads that need to access a single InitialContext instance concurrently should synchronize amongst themselves and provide the necessary locking."

    Question is, does Sun know something I don't (eg. that lookup is an immutable call and therefore threadsafe, which the javadoc does not seem to imply), or is the Sun implementation incorrect?

    BTW. there is a fairly easy workaround which is to call:

      Object objRef = ((InitialContext) initialContext.lookup((Name) null)).lookup(name);

    as ic.lookup((Name) null) will return a new instance that is local to your current thread, but is it necessary to do so?

    all help clearing this up most appreciated,

    cheers,
    - jed.
  2. Looking at the code implementation from the provided URL, Sun's example implementation appears correct.

    Notice that their ServiceLocator example is a Singleton class that uses a private constructor. This private constructor will initialize or create the InitialContext. Only this initialization or the assignment of the InitialContext should be synchronized or thread safe. Once it's initialized multiple threads may safely use it to perform lookups without any synchronizations. Now their example was able to avoid the use of synchronization by only having the private constructor called in a static initializer method that will safely call the private constructor the very first time the class gets initialized. So no synchronization is needed here.

    Now assuming that what they provided is a partial implementation and that a full implementation will never re-assign or re-set the InitialContext they could have marked "InitialContext ic" final so that once it was set it's clear that it's never going to be changed.
  3. I'm not 100% convinced that the ServiceLocator is a singleton in an app server with multiple class loaders. I'm really very surprised to see Sun pushing a design that is dependant upon class loading semantics of individual app servers.

    Wouldnt the static "me" be only static within a single loader?

    Dave Wolf
    Personified Technologies LLC
  4. Your point about using the Singleton pattern in an app server environment is correct.

    However, while this is surely not the way to implement an uniformly-ascending-sequence numerator, multiple class loaders are not a critical or a business issue in this example: you *can* have multiple ServiceLocators. Sun's design seems like an "ostrich design", but, in fact, is sufficient for the deed.
  5. Right but you couldn't have multiple service locators and still maintain thread-safety on the un-safe InitialContext?

    As you show, the ONLY way to assure that is to wrap the access to the InitialContext within synchronized blocks.

    Which simply points back to my original opinion which is that the pattern, as supplied by SUN, is fundamentally flawed.

    Dave Wolf
    Personified Technologies LLC
  6. First off... multiple class loeaders on the App Server will not prevent one from implementating a Singleton (Singleton being defined as one instance per JVM).

    As I originally stated, the initialContext.lookup(name)does not require synchronization. Only the manipulation of the InitialContext requires synchronizing --- not the use of it. Lookup is not manipulating it.

    To say that initialContext.lookup(name) is not thread safe would be the same thing as saying that the below example would not be thread safe.

    public MyClass {
      private static final String m_Hello = "Hello";

      public static String sayHello(String name) {
        StringBuffer sb = new StringBuffer(m_Hello);
        sb.append(": " + name);
        return sb.toString();
      }
    }

    Now I think that we would all agree that the above example is thread-safe because each thread operates within a separate memory allocation. So each thread can safely enter in a different String name and get back a different sb.toString() result. The only thing that the two threads share is m_Hello. So as long as any manipulation of m_Hello is synchronized (or immutable), sayHello can be considered thread-safe. Similarly, initialContext.lookup(name) should act the same way.

    If you're going to use a ServiceLocator.. everybody and their brother will be calling the lookup method within it. So this part needs to be screaming -- so for your own benefit, don't synchronize it.
  7. Larry, what you are saying makes sense to me, and was always the way I figured it would have to be for the pattern to be correct. My major question with this was always really about the thread-safety of lookup(...), and the fact that the javadoc is not at all clear on this point. Can you be sure that lookup() doesn't manipulate any internal state (doesn't manipulate the ctx)? The contract doesn't explicitly state this, and I guess it is possible that some implementations are not necessarily thread-safe.

    btw. your example below is a common-sense implementation, but you can be sure that if you gave 100 programmers the same contract to implement (without an explicit thread-safe demand), at least some of them would find a way to implement it unsafely ;-)

    thanks,
    - jed.
  8. Another point to add to this is that the javadoc is not clear that any kind of concurrent access is supported, whether it be mutable or not, indeed it explicitly states that it is not, hence the confusion. To reiterate:

    "...Threads that need to access a single InitialContext instance concurrently should synchronize amongst themselves and provide the necessary locking. "

    Note: "... that need to access ..."

    It does mention manipulating a bit before that, just to confuse the issue ;-)

    cheers,
    - jed.
  9. Jed's original question drew more of my attention, I'm looking into it, also. Any new ideas/threads Jed?

    BTW, regarding your "workaround": what if two threads enter "ic.lookup((Name)null)" at the same time? You still have the run-condition.

    I think you should really synchronize on the initial context everywhere you have to lookup a name, as in:

     ...
     Object objRef;
     syncronized(initialContext) {
       objRef = initialContext.lookup(name);
     }
     ...
  10. Sorry Ferhat, no new ideas, too busy ;-)

    > BTW, regarding your "workaround": what if two threads enter "ic.lookup((Name)null)" at the same time? You still have the run-condition.

    Yeah, if we believe that lookup() is fundamentally unthreadsafe then you are probably correct, there is no way to access an InitialContext concurrently (at all!), and synchronization should be done as you suggest.

    cheers,
    - jed.