Tom White's Blog

Are your beans thread-safe?

[Update: changed wording per comments to fix error.]

Dependency injection is pretty well established these days, with plenty of Inversion of Control containers available to manage your beans. I'm currently reading Java Concurrency in Practice by Brian Goetz et al, which got me thinking about the thread-safety of large object graphs managed by IoC containers.

In most applications I've seen, the common usage pattern is to use dependency injection to wire up your object graph in one go when the application starts. After that the application uses the object graph, and effectively treats it as being immutable. For example, in an application for buying books we might have a BookStore object that is given a BookFinder so it can find whether a particular book is available. The BookFinder is created at the beginning of the application and never changes. It is common to code this using setter injection:

public class BookFinder {

    // BookFinder's business methods...

}

public class BookStore {
    private BookFinder bookFinder;

    public void setBookFinder(BookFinder bookFinder) {
        this.bookFinder = bookFinder;
    }

    // BookStore's business methods which use bookFinder...

}

The problem is that it's not clear that this code is thread-safe. Unless the container publishes the BookStore bean safely then there is no guarantee that other threads will see the value of the BookFinder bean. If this seems weird then that's because it is. To quote Java Concurrency in Practice (p33): "In general, there is no guarantee that the reading thread will see a value written by another thread on a timely basis, or even at all."

Thankfully, this is easy to fix. We can mark the bookFinder instance as volatile to ensure its visibility to other threads:

public class BookStore {
    private volatile BookFinder bookFinder;

    public void setBookFinder(BookFinder bookFinder) {
        this.bookFinder = bookFinder;
    }

    // BookStore's business methods which use bookFinder...

}

Alternatively, we can use constructor injection and final fields. This works because the Java Memory Model makes special guarantees about the safety of final fields.

public class BookStore {
    private final BookFinder bookFinder;

    public BookStore(BookFinder bookFinder) {
        this.bookFinder = bookFinder;
    }

    // BookStore's business methods which use bookFinder...

}

The Future

So could the IoC container providers take steps to ensure that application developers don't have to worry about thread safety in wiring up object graphs? Possibly, although at the time of writing it is less than clear whether the popular containers do or not. Tim Peierls, one of the authors of Java Concurrency in Practice, wrote in a email to me that "Until it's proven safe, I have adopted a rule that all such setter-injected effectively immutable fields must be volatile." He also suggested marking the field with an annotation, such as @WriteOnce, so that when your container's behaviour is clarified it is easy to go through code and remove the volatile modifier.

Comments

• this is a perfect example of why setter injection is flawed compared to constructor injection. if a property IS immutable, why have a setter method?

  Posted by: wireframe on September 21, 2006 at 02:54 PM

• 
  Responding to the comment about why one would ever use setter injection in the first place:

  
  Constructor injection isn't always the best way.

  There might be so many combinations of fields that providing a constructor for each combination is impractical.

  
  The dependency injection might have a side effect of allowing the this reference to escape (e.g., via listener registration), which is a no-no in the constructor.

  
  The fields might be hard to document nicely in the javadoc comment for the constructor, where the setter injected version would allow each field to have its own method comment.

  
  A bean might need to implement an interface that already provides the obvious setter methods.

  By all means use constructor injection for immutable fields if it is convenient, but don't go through contortions if it is more natural to use setter injection. Just be aware of the concurrency risk that Tom's article describes.
  

  Posted by: tpeierls on September 21, 2006 at 09:58 PM

• > Even if the container publishes the BookStore bean safely then there is no guarantee that other threads will see the value of the BookFinder bean

  ARGH. Are you calling #setBookFinder after the publishing the bookstore? That surely wouldn't make sense. So the happens-before relationship is: create book finder --samethread--> setBookFinder --samethread--> publish bookstore --synchronization--> read bookstore. In other words, bookfinder will always be visible.

  Matthias
  

  Posted by: mernst on September 21, 2006 at 11:13 PM

• I think there should be a way to "seal" a bean once it has been totally built with setters, so that any try to modifiy it once it has been marked as "sealed" would throw a RuntimeException (an IllegalStateException or something like this).

  It could be a method of the bean that changes a boolean private field, but it would be a bit tedious to put code in every setter method to enforce this behaviour, something like:

  public void setMyProperty(String value){
  if (this.isSealed){
  throw new IllegalStateException("Object sealed cannot be modified");
  }
  this.myProperty = value;
  }

  
  Xavi

  Posted by: xmirog on September 22, 2006 at 12:43 AM

• 
  Matthias,

  No, I'm not talking about calling setBookFinder after the BookStore bean is published. The chain of events that you describe is fine, except it's not clear who provides the sychronization. My point is that the IoC container typically does not guarantee this, so until it does you should take your own measures to ensure the thread safety of your system. Marking the bookFinder field as volatile is one way to do this. The Java Concurrency in Practice has more on this, or have a look at Fixing the Java Memory Model, Part 2.

  Tom

  
  

  Posted by: tomwhite on September 22, 2006 at 01:14 AM

• Tom,

  didn't you write "Even if the container publishes the BookStore bean safely ..."? That entails some sort of synchronization or use of volatile for me.

  I say you're attacking the problem at the wrong end. I see two options: either the container constructs the beans upfront. Then you simply need to make sure that you publish the container safely afterwards. Problem solved. If the container constructs the beans on-demand but caches them then it's either the container's job to garantuee the safety of the publication (for example, Spring uses a synchronized map for its singleton beans); if it doesn't, it is not safe to access it multi-threaded anyway and you should synchronize access to the container. If your container has come up with the equivalent of DCL on the map of beans here (unsynchronized lookup first, sync if not found), I would let go of such an implementation immediately. Don't start messing with the beans to safeguard against a broken container.

  Cheers, Matthias
  

  Posted by: mernst on September 22, 2006 at 01:32 AM

• 
  Matthias,

  I did write that, and you are right - I have changed it to say "Unless the container publishes the BookStore bean safely then there is no guarantee..." which captures what I meant. Sorry about that.

  The problem here is that the documentation of IoC containers don't talk about their thread safety aspects - if they did then bean writers could have more confidence.

  Tom

  
  

  Posted by: tomwhite on September 22, 2006 at 02:28 AM

• This post is total nonsense. The Spring container - the leading IoC container - makes sure that bean creation and the calls to setters is perfectly thread-safe, both for singletons and prototypes.

  I suggest to the author to get the facts right before defaming "IoC containers".

  Posted by: devijvers on September 22, 2006 at 04:36 AM

• The problem here is that the documentation of IoC containers don't talk about their thread safety aspects - if they did then bean writers could have more confidence.

  Come again? You as an author don't know that Servlet Listeners are started before Servlets are available to handle requests? Get out of here.

  Posted by: devijvers on September 22, 2006 at 04:44 AM

• Woa, Steven. The "leading" folks, especially, should relax and show some greatness. No good advertisement for Spring.
  

  Posted by: mernst on September 22, 2006 at 04:53 AM

• Xavi - 'sealing' objects like that is the point of the Essence pattern

  You can see examples of this in practice in the code for Acegi, eg in the LdapUserDetailsImpl class

  Posted by: bazzargh on September 22, 2006 at 05:34 AM

• I'm sorry if I upset anyone. Many people get confused about IoC in part because nonsense like this gets spread and is taken for granted.People, there is no issue with thread-safety. The Servlet specifications and the way IoC containers like Spring integrate with Servlet engines makes sure you as a developer don't have to worry about anything.

  Posted by: devijvers on September 22, 2006 at 07:14 AM

• 
  Matthias wrote: "Don't start messing with the beans to safeguard against a broken container."

  
  It's hard to know whether a container is broken. Until you know for sure, better safe than sorry. I don't understand why you are reluctant to take simple measures that at worst impose a small performance hit (probably negligible in all practical settings) and at best keep your programs from being horribly broken.

  
  I've heard the claim before that Spring beans are always retrieved from a thread-safe collection. I think it's quite likely that Spring bean publication is safe. But whenever I've tried to satisfy myself that this is the case, I've run into a lot of code, all lacking a documented synchronization policy (that I could see). From a practical programming standpoint, Spring is broken because of this absence.

  
  Please don't interpret this as an attack on Spring or its creators. I love Spring. I use it happily in production code. I just want this issue resolved, either through better documentation or with code changes, before I take the volatile keyword off my write-once setter-injected fields.

  
  Tim Peierls

  Posted by: tpeierls on September 22, 2006 at 07:33 AM

• Tim,

  I see. The reasoning that let's me sleep at night is: a Spring ApplicationContext pre-instantiates all singleton beans when the container is created. This is done in the #init of my servlet (context) or at the start of my application. After that, Spring will not mess with my beans. The container is then safely published to all application threads (either I start the threads later, or the servlet container synchronizes the webapp startup). Therefore, Spring's synchronization policy actually is of no concern here (note that I do not use lazy-init beans). It's like populating a plain old hashmap and then publishing it safely.

  Cheers, Matthias

  Posted by: mernst on September 22, 2006 at 07:58 AM

• Matthias,

  I'm glad you can sleep at night, but I'm not sure you should. :-) Unless you are in the enviable position of being the only person who will ever maintain your code (and maybe even then), the assumptions that you are relying on -- eager init singleton beans in a servlet -- could change out from under you.

  
  Another way to look at it is that IoC enables and encourages you to write your beans to be container-agnostic, so why would you want to limit the reuse of a bean to containers with particular bean publication guarantees? Make the bean thread-safe from the start and it can be reused in any container.

  Tim

  Posted by: tpeierls on September 22, 2006 at 08:23 AM

• I get grumpy if I don't sleep. Making all my beans threadsafe as proposed here would be a nightmare to maintain IMHO. The mere thought that any one of my fields were missing a volatile would keep me awake. Either the rule is simple (documented pre-instantiate-> publish OR lazy-init with container-provided documented safety) or I'm not gonna use it. And Tom, I hope you do not mind us abusing your post here... I think we've pretty much exchanged our views, anyway.
  Great discussion, thanks. Matthias
  

  Posted by: mernst on September 22, 2006 at 08:46 AM

• The Spring container does more than only creating beans upfront. It actually has an extensive bean lifecycle which includes dependency injection of setters. Only when this entire lifecycle has been completed beans become available (this is sufficiently close to the truth in this discussion).

  Spring has a detection mechanism that will prevent *singleton* beans from being created concurrently and it avoids concurrent setter injection altogether (prototype beans do not face thread-safety issues). This combined with the fact that the Spring container is a shared factory instance that is created when the Servlet container starts (in case of web applications) makes sure that it's impossible to get thread-safety issues when doing setter injection.
  

  Posted by: devijvers on September 22, 2006 at 08:48 AM

• 
  I understand that the intention is that the Spring bean lifecycle management provides safe publication. What is missing is a documented concurrency policy that makes me feel confident this intention is correctly implemented and will stay correct in the face of subsequent code maintenance.

  
  I found a concurrency bug in Spring recently, a race condition in ResponseTimeMonitorImpl (can you spot it?). It is extremely minor, but it means one of two things: either the author didn't realize there was any concurrency issue with the code or the author understood it but failed to document that understanding. Either way, the strong implication is that there could be other, more serious problems elsewhere in Spring.

  
  This combined with the fact that the Spring container is a shared factory instance that is created when the Servlet container starts (in case of web applications) makes sure that it's impossible to get thread-safety issues when doing setter injection.

  
  I hope you're right, but if there's one thing I have learned about concurrency, it's that you should never make claims like this. Even the experts make mistakes. :-)

  Tim Peierls

  Posted by: tpeierls on September 22, 2006 at 09:54 AM

• Hmmm, you seem a little bit paranoid :-) Sure there are concurrency issues with Spring and I've discovered one myself recently. However, these issues are caused by Spring code, not by developer code.

  The only people that need to be aware of more details about the bean lifecycle in order to prevent certain cases of concurrency issues are people that use Spring to build their own frameworks.

  People that build application with Spring (99% of Spring users) don't need to worry about concurrency issues.

  I hope you're right, but if there's one thing I have learned about concurrency, it's that you should never make claims like this. Even the experts make mistakes. :-)

  Sure I'm right. Take a look at the Servlet specifications, Spring's ContextLoaderListener class and Spring's ApplicationContext contract.

  Posted by: devijvers on September 22, 2006 at 10:16 AM

• 
  Hmmm, you seem a little bit paranoid :-)

  
  Funny, I was going to say you seem a little too sanguine! :-)

  
  Sure I'm right. Take a look at the Servlet specifications, Spring's ContextLoaderListener class and Spring's ApplicationContext contract.

  
  I have looked at the Servlet specifications. Read Section 4.5.1 of Java Concurrency in Practice about how to deal with frustratingly vague specifications.

  
  The javadocs for ContextLoadedListener and ApplicationContext contain
  no mention of thread-safety properties. Read the rest of Section 4.5 to get a better sense of what I mean by a properly documented concurrency policy.
  

  Posted by: tpeierls on September 22, 2006 at 11:37 AM

• Well, for me there is no urgency or thread-safety issue. The integration with Servlet containers as implemetend by Spring is proven and trusted upon by millions. Notice that the only Spring concurrency issues that have been reported are related to cyclic dependencies. I still don't see what the issue is. And what kind of statement do you expect?

  Posted by: devijvers on September 22, 2006 at 12:08 PM

• In my opinion Thread Safety to most extent is a topic for experts, and should be treated on a Frame Work level, so as to assure this just as garbage collection to the general programmer that its managed in the background. So that he/she can focus on the business logic rather than studying tuns of technical product manuals.

  I have recently programmed a remove Hibernate like implementation of EJB3, and yes although Threading could be an issue in the future
  the framework configuration files are loaded in the servlet init method. If any code would access the object models used before this method it would be a programming error made by the programmer(why because the manuals and code only give access to outside proramming code after the init method have been called). Being a framework I still would not expect any company programmers using it to be responsible for thread errors. This should be solely an issue for the Frame Work.

  All said threading is not an issue for the majority of programmers and for that case for the majority of programming tasks.

  Multi Threading problems ussually arise from a design/implementation error in which either resources and set once only objects are not loaded before any processing by outside code is done, or in which the implementation does operate code before the set once only objects have been initialised.

  Posted by: gerrit_jvv on September 22, 2006 at 06:07 PM

• 
  If I can sum up, I think everyone is agreed that it should be the container's job to provide a thread-safe environment for beans, so bean writers don't have to worry about these aspects. While the jury is still out as to whether current IoC containers (such as Spring) do provide a thread-safe environment, there is disagreement over whether bean writers should code defensively (by using volatile instance fields with setter injection, for example) or not worry about it (since it's very unlikely to be a problem in practice).

  And finally, I do think documentation is the real issue here. As Java Concurrency in Practice points out - the thread safety aspects of frameworks and APIs are very commonly under-documented (even in the core Java APIs), a situation which is increasingly dangerous in the world of multi-core and multi-processor machines. Hopefully, we'll see more debate on these matters.

  Thanks for the discussion everyone (even if it was about "nonsense" :-)!

  Tom

  
  

  Posted by: tomwhite on September 23, 2006 at 02:39 AM

• 
  I don't know Spring that much, but as far as I can read at Erik Wiersma's blog it seems all beans in an application context are singletons.

  This is, constant references to objects.

  So I don't think they're very thread-safe.

  Erik's blog has some approches to synchronization.

  Cheers,
  Antonio
  

  Posted by: vieiro on September 29, 2006 at 02:49 AM

• bazzargh,

  thank you for your comment, the Essence pattern is what I was talking about. I didn't know it actually had a name, thanks a lot.

  Xavi
  

  Posted by: xmirog on October 02, 2006 at 03:34 AM

• I just came across an example of un-threadsafe code in core Spring 2.0, in the org.springframework.context.access.ContextBeanFactoryReference class. To be fair, the javadoc does say that the API for this class doesn't guarantee thread-safety, but then it does contain a synchronized section to make it so.

  The trouble is, it isn't, even with the synchronized block. The getFactory() method is still unsynchronized, and the VM does not guarantee consistency with the release() method.

  Not Spring bashing here, just pointing out an example.

  Posted by: skaffman on October 24, 2006 at 12:12 AM

• Tom, I think Tim Peierls is right. But according to current JMM, an 'effectively immutable field' can't be threadsafe without a synchronization action between the producer (publisher) thread and any consumer threads. I call such fields 'effectively final'. You may want to look at http://negev.wordpress.com/java-memory-brief.

  Posted by: peter_kehl on January 12, 2008 at 02:40 AM

This work is licensed under a Creative Commons License.