Concurrency issues in Restlet

Issue #368 in the Restlet issue tracker was originally focused on the Guard class. Since then, I've taken a look at the rest of the org.restlet package and found that the fields of most classes are accessed without appropriate synchronization. [Update 2008-Feb-6: All of these have been fixed as of Restlet 1.1 M1.]

Most of these problems would disappear if the fields could be final, but that would mean taking away setter-injectability.

There is hope, however: Methods that are used only for setter injection (and not, for example, used to reconfigure an instance dynamically), should be documented as setter-injection-only, meaning they must not be called after the instance has been (safely) published. Fields set by such methods get their values before publication, and those values never change; such fields are effectively immutable. I don't know of any standard documentation conventions for this, but the important thing is to decide for each non-final, non-volatile field whether it is truly mutable or only setter-injected before publication.

I have a long-standing gripe about the Spring documentation that it doesn't state, at least not in terms that I recognize, under what conditions beans are safely published. The vanilla use cases (standard ApplicationContext implementations) are almost certainly fine, but if you have any doubts about whether your dependency injection framework can guarantee safe publication, then you should either guard your setter-injected fields with a lock or make them volatile. I have a personal preference for using volatile in this case, but most of my Java Concurrency in Practice co-authors have the opposite preference.

Even if you're fairly confident about your DI framework's guarantees, it is never wrong to guard field access or make a field volatile, only potentially wasteful. For a framework like Restlet that should be usable in any DI context, I think it would be prudent to assume the worst of that context.

But is setter injection really necessary? Most of the injected fields are of distinct types, so constructor injection in Spring would be straightforward. Getting rid of setter injection entirely would allow most, if not all, fields to be final, which would greatly reduce the risk of concurrency problems in Restlet.

Babel

Jeffrey Sanzel writes (referring to this posting):

Here are the words I understood on your blog:

blog
Tim
Peierls (to an extent)
A (in context)
of (in context)
There
All

Basic rules for @ThreadSafe classes

Some rules for classes to be annotated with @ThreadSafe:

1. All non-final, non-volatile fields (and final references to non-thread-safe state) must be annotated with @GuardedBy(x), where x is usually this, meaning that all access to the field -- reads and writes -- is performed within a synchronized (this) block or synchronized method. [This is probably the hardest rule for people to accept; it's not intuitive that reads must be performed with a lock held. See amplification below.]
   


2. If two fields annotated with @GuardedBy participate in the same class invariant, they must be annotated with the same argument to @GuardedBy. [This is trivially satisfied if you only use @GuardedBy("this").]
   


3. All compound actions with atomic semantics (e.g., check-then-act and read-modify-write sequences) must be performed completely within a synchronized block or method. [This is what many people incorrectly think is the only reason to use synchronized.]
   


4. Do not make calls to code whose synchronization properties you don't know or control from inside a synchronized block or method; prefer open calls (calls made with no locks held). [This is a strategy for deadlock avoidance.]
   


5. Prefer final fields. For collection fields, prefer thread-safe collections. (But don't bother using a thread-safe collection if the field needs to be @GuardedBy for some other reason.)
   


6. Prefer final atomic variables to volatiles. Volatile fields cannot be modified atomically without a synchronized block.
   


7. There need be no special relationship between x and y in "@GuardedBy(x) Type y;" as long as all access to y is made while holding x's lock.
   


8. Don't be too clever: even if your clever reasoning is correct right now, it will be harder in the future for others (and you) to understand and maintain the code that relies on that reasoning.
   

Amplification

Brian Goetz suggests that I make this point more strongly: Not only do you need to declare such fields @GuardedBy(lock), you have to acquire lock every time you access the field in any way.

Concurrency discussion in Restlet community

I started a thread on concurrency issues in the Restlet framework, and it led to a several people saying nice things about Java Concurrency in Practice.

Jérôme Louvel's responsiveness to these issues is gratifying. A far cry from the huffiness I provoked when I had the temerity to question the concurrency guarantees in Spring!

Java Concurrency in Practice news

I'm told that Java Concurrency in Practice is going into its fifth printing!

Custom Matcher and MethodInterceptor for DWR-Guice

I wanted a way to intercept calls to public methods of a remoted interface so I could replace my use of bindFilter with bindInterceptor, as described in an earlier posting.

At first I thought I could just use the static methods in Matchers:

bindInterceptor(
  only(MyService.class), 
  any(), 
  myInterceptor
);

This doesn't intercept anything, because MyService is an interface. What I needed was to intercept calls to a method of a subclass of MyService for which there exists a method of the same name and argument types in MyService.

bindInterceptor(
  subclassesOf(MyService.class), 
  declaredBy(MyService.class),
  myInterceptor
);

The subclassesOf matcher is already provided in com.google.inject.matcher.Matchers, but I had to roll my own declaredBy method matcher. Here's the heart of the implementation (cls is a final Class<?> field, initialized from the argument to declaredBy):

public boolean matches(Method method) {
  try {
    // Matches if the method is from a subclass 
    // of the given class (or the class itself) 
    // and the given class declares a method
    // with the same name and parameter types. 
    if (cls.isAssignableFrom(
             method.getDeclaringClass())) {
      // Return value of getDeclaredMethod 
      // is ignored. It throws an exception 
      // if the method is not found.
      cls.getDeclaredMethod(
        method.getName(), 
        method.getParameterTypes());
      return true;
    }
    // fall through
  } catch (NoSuchMethodException e) {
    // fall through
  } catch (SecurityException e) {
    // fall through
  }
  return false;
}

This still didn't work quite right; the implementation of one method called another public method of the interface, and that call was intercepted even though it wasn't a remote call. To mimic AjaxFilter, I want to able to intercept only the outermost call. I wrote a MethodInterceptor-decorator:

public class OutermostCallInterceptor 
    implements MethodInterceptor {
  /**
   * Decorates a MethodInterceptor so that only the 
   * outermost invocation using that interceptor will 
   * be intercepted and nested invocations willbe 
   * ignored.
   */ 
  public static MethodInterceptor outermostCall(
      MethodInterceptor interceptor) {
    return new OutermostCallInterceptor(interceptor);
  }

  /** Ensure underlying interceptor is injected. */
  @Inject void injectInterceptor(Injector injector) {
    injector.injectMembers(interceptor);
  }

  public Object invoke(MethodInvocation invocation) 
      throws Throwable {
    int savedCount = count.get();
    count.set(savedCount + 1);
    try {
        if (count.get() > 1) 
          return invocation.proceed();
        else 
          return interceptor.invoke(invocation);
    } finally {
      count.set(savedCount);
    }
  }

  private OutermostCallInterceptor(
      MethodInterceptor interceptor) {
    this.interceptor = interceptor;
  }

  private final MethodInterceptor interceptor;

  private final ThreadLocal count = 
    new ThreadLocal() {
      @Override protected Integer initialValue() {
        return 0;
      }
    };
}

So now my binding looks like this:

bindInterceptor(
  subclassesOf(MyService.class), 
  declaredBy(MyService.class),
  outermostCall(myInterceptor)
);

and myInterceptor is injected. It's not as compact as using bindFilter, but I can apply multiple interceptors without having to resort to FluentConfigurator. The use of subclassesOf is probably redundant, since declaredBy checks that the method is from a subclass of MyService, but I think it helps to clarify what's going on.

Replacing AjaxFilter with MethodInterceptor

I wrote the previous posting without referring to DWR-Guice integration but in practice I extended org.directwebremoting.guice.AbstractDwrModule, not com.google.inject.AbstractModule.

For DWR-Guice users, the nice thing about injectable method interceptors is that you can use them to replace most uses of AjaxFilter, which is a good thing because the AjaxFilter support in the DWR-Guice stuff is weak (mea culpa).

I had to write some support utilities to get the right effect, but now I can do something like this:

bindInterceptor(
 subclassesOf(VulnerableService.class), // class matcher
 declaredBy(VulnerableService.class),   // method matcher
 // prevent nested interception
 outermostCall(new AuthenticationInterceptor()),
 outermostCall(new AuthorizationInterceptor())
);

and AuthenticationInterceptor is injected, so that it can look like
this:

class AuthenticationInterceptor 
    implements MethodInterceptor {
 @Inject Provider<RequestParameters> reqParmsProvider;
 @Inject Provider<AuthenticationService> authSvcProvider;
 public Object invoke(MethodInvocation invocation) 
     throws Throwable {
   RequestParameters reqParms = reqParmsProvider.get();
   AuthenticationService authSvc = authSvcProvider.get();
   if (authSvc.authenticate(reqParms))
     return invocation.proceed();
   else
     throw new AuthenticationException(reqParms);
 }
}

The subclassesOf matcher is part of Guice. The declaredBy matcher and the outermostCall interceptor decorator are custom implementations that I'll describe in a later posting.

Injecting method interceptors in Guice 1.0

Guice 1.1 will probably have injected method interceptors, but until then those of us who need this kind of thing will have to roll our own.

[Update 2009-1-3] Guice 2 has (or will have, depending on how you view its release status) a requestInjection facility.

The way I've been doing it is similar to the way Stuart McCulloch suggested, but more automatic: I extend AbstractModule by

1. adding a registerForInjection method that takes a varargs array of objects that should be injected and
2. overriding bindInterceptor to call registerForInjection(interceptors) before calling the superclass implementation.

The implementation of registerForInjection adds the elements of the array argument to a set of objects to be injected. There is a private method, injectRegisteredObjects, marked with @Inject; all it does is call Injector.injectMembers on every element of this set.

To ensure that injectRegisteredObjects is called, I bind the module class to the module instance. Since I want to be able to do this for every instance of the module, I bind with a unique annotation instance each time. The private method ensureSelfInjection does this binding once only for each module instance, so I can safely call it in registerForInjection.

Here's the code:

/**

 * An extension of AbstractModule that provides
* support for member injection of instances
* constructed at bind-time; in particular,
* itself and MethodInterceptors.
*/
public abstract class ExtendedModule extends AbstractModule {

 /**
  * Overridden version of bindInterceptor that,
  * in addition to the standard behavior,
  * arranges for field and method injection of
  * each MethodInterceptor in {@code interceptors}.
  */
 @Override public void bindInterceptor(Matcher<? super Class<?>> classMatcher,
                                       Matcher<? super Method> methodMatcher,
                                       MethodInterceptor... interceptors) {
   registerForInjection(interceptors);
   super.bindInterceptor(classMatcher, methodMatcher, interceptors);
 }

 /**
  * Arranges for this module and each of the given
  * objects (if any) to be field and method injected
  * when the Injector is created. It is safe to call
  * this method more than once, and it is safe
  * to call it more than once on the same object(s).
  */
 protected <T> void registerForInjection(T... objects) {
   ensureSelfInjection();
   if (objects != null) {
     for (T object : objects) {
       if (object != null) toBeInjected.add(object);
     }
   }
 }

 @Inject private void injectRegisteredObjects(Injector injector) {
   for (Object injectee : toBeInjected) {
     injector.injectMembers(injectee);
   }
 }

 private void ensureSelfInjection() {
   if (!selfInjectionInitialized) {
     bind(ExtendedModule.class)
       .annotatedWith(getUniqueAnnotation())
       .toInstance(this);
     selfInjectionInitialized = true;
   }
 }

 private final Set<Object> toBeInjected = new HashSet<Object>();
  
 private boolean selfInjectionInitialized = false;  

 /**
  * Hack to ensure unique Keys for binding different
  * instances of ExtendedModule. The prefix is chosen
  * to reduce the chances of a conflict with some other
  * use of @Named. A better solution would be to invent
  * an Annotation for just this purpose.
  */
 private static Annotation getUniqueAnnotation() {
   return named("ExtendedModule-" + count.incrementAndGet());
 }

 private static final AtomicInteger count = new AtomicInteger();
}

There's a follow-up posting on DWR-Guice applications.

Children's show opening

The Fairy Princess, a children's show that Jeffrey E. Sanzel and I wrote, opened today at Theatre Three in Port Jefferson. It runs Fridays and Saturdays at 11am throughout July.

Fifth of July

It would have been more appropriate to post this yesterday, I suppose:

Michele Oliver blogged about the song "This is America", on which Ilene and I can be heard as part of the backing chorus near the end of the song. It was recorded in 2001, partly in response to the 9/11 attacks.

A Guice utility for binding to legacy classes

You're going along, happily Guicing your world, when you run into a snag. There's a class Legacy that you'd like to be able to bind things to, but you can't edit the source to add the @Inject annotations. It looks like this:

public class Legacy implements SomeInterface {
 public Legacy(LegacyDependency dep, String str) {
   ...
 }
 public void initialize(LegacyConfig cfg) {
   ...
 }
 ...
}

And you wish it looked like this:

public class Legacy implements SomeInterface {
 @Inject public Legacy(LegacyDependency dep,
                       @MyAnnotation String str) {
   ...
 }
 @Inject public void initialize(LegacyConfig cfg) {
   ...
 }
 ...
}

You're used to Guice by now, so right away you think, "No problem, I'll use a Provider!"

public class LegacyProvider
     implements Provider<Legacy> {

 public Legacy get() {
   Legacy result = new Legacy(dep, str);
   result.initialize(cfg);
   return result;
 }

 @Inject LegacyDependency dep;
 @Inject @MyAnnotation String str;
 @Inject LegacyConfig cfg;
}

...

 protected void configure() {
   bind(SomeInterface.class)
     .toProvider(LegacyProvider.class)
     .in(WEB_APPLICATION_SCOPE);
 }

So you've written a Provider<Legacy>, and it works. But now you have this extra class to maintain. It's not complicated, in fact it's quite trivial. It seems like something you shouldn't have had to write. You'd rather write something like this:

 bind(SomeInterface.class)
   .toProvider(

     fromConstructor(Legacy.class,
       Key.get(LegacyDependency.class),
       Key.get(String.class, MyAnnotation.class))

     .injecting("initialize",
       Key.get(LegacyConfig.class))

     )

   .in(WEB_APPLICATION_SCOPE);

If this looks like something you want, check out my implementation in the DWR-Guice integration sources. The javadocs are there, too, but they aren't complete. It doesn't have anything to do with DWR, but it seems like a nice convenience to provide.

There are also several flavors of factory-method-based providers, so you don't have to write a special provider just to call a factory method.

I wrote it because even though I've been able to jettison a lot of baggage as I migrate from Spring to Guice, there are still Spring things I want to keep using, like JavaMailSender. I don't want to have to write provider implementations for each one.

Guice support in DWR

Guice is the hot new dependency injection framework from Bob Lee and others at Google. DWR is a system for remoting Java server-side objects as JavaScript client-side objects in a way that is natural on both the client and server sides.

I'd been using DWR with Spring to wire together all the components of the Seat Yourself on-line ticket sales web application for almost a year and I was pretty happy with the combination. Then a few weeks ago, just as I was considering how to make the webapp more easily configurable, to support rapid administration and self-configuration of many customers, Josh Bloch suggested I take a look at Guice, and I suddenly realized just how fragile and unmaintainable was all that XML I had written for DWR and Spring.

Of course I could use Guice without integrating it with DWR, but one of the things that had always bothered me about DWR (and many other tools, not just DWR) was that everything -- remoted objects, data transfer objects, extension classes -- has to be concrete and public, with a public parameterless constructor, and everything works through setter injection, which makes things difficult. I want to work in terms of interfaces, with implementation classes that have final fields for injected state.

Guice looked like a way to have it all: easy remoting with type-safe (pure Java) configuration. I wrote a Guice integration package for DWR, and Joe Walker signed me on as a DWR contributor so I can help maintain it as part of the DWR distribution. The package Javadoc describes how to use it along with a simple example.

I'm still in the early stages of applying it to the Seat Yourself web sales application, but already the power of Guice has made things simpler. Each customer (where a customer is ticket-selling entity, like a theater) is associated with a domain where all their data -- seat charts, performance information, pricing information, patrons and their reservations, and holds on seats -- is maintained. When a user visits the web store to buy tickets, the link contains a domain name, e.g., "t3" for Theatre Three. Before Guice support, I had devised a complicated system using reflection and proxying to route the user request to the appropriate service object. It was so involved that I had trouble explaining it to my partners, both smart people. Not being able to explain something is a bad code smell.

With Guice-injected remoted objects, I can define a new scope, domain scope, where the details of providing the appropriate domain-specific service object are neatly encapsulated and managed by Guice. All I have to do is write something like this in my binding code:

bindRemotedAs("Tix", WebstoreService.class) // interface
  .to(WebstoreServiceImpl.class)            // impl
  .in(DomainScopes.DOMAIN);                 // scope

where the interface looks like this:

public interface WebstoreService {
  PerformanceAvailability getPerformanceAvailability();
  ...
}

Only the methods of WebstoreService are exposed to the world, even though the implementation class might have other public methods. With the Guice support, I don't need to specify which methods to expose. As usual with DWR, on the JavaScript side I can write things like:

Tix.getPerformanceAvailability({
  callback : function(availability) {
    notifyPerformanceAvailabilityReceived(availability)
  },
  exceptionHandler : ...
});

Because Guice has Spring integration, I can migrate my existing Spring-based code smoothly. Because it has JNDI integration, I can rewrite the code that manages the domain-specific configuration to use whatever LDAP back-end I like, instead of having to maintain an assortment of XML files and registry settings. We've already had problems with syntax errors in the XML configuration files, so this will save us a lot of heartache.

Guice also has support for AOP Alliance method interceptors, which means that on most of the occasions when I would feel most tempted to use AspectJ, I can stick to pure Java. Not that I have anything against AspectJ, but it's a different language and one more thing to worry about. I want to save it for the occasions when I really need it (as was the case for another submodule of our project).

Blurb in Yale Alumni Magazine

The class notes section of the current issue of the Yale Alumni Magazine includes some notes I submitted in the fall.

Tim Peierls (JE) (tim@peierls.net) writes that 2006 was exceptionally busy, with his hand in a wide range of diverse ventures: "I helped produce fellow JEer Josh Rosenblum's musical show, Bush is Bad; the title says it all. I co-wrote a book, Java Concurrency in Practice (Addison-Wesley). If the title makes sense to you, then you need the book; if not, then ... not. I co-produced (with Gary William Friedman) an album by jazz vocalist Stevie Holland, More Than Words Can Say. And I developed an online ticket sales application that went into production in November. See www.SeatYourself.biz for details."