 |
Are your beans thread-safe?
Posted by tomwhite on September 21, 2006 at 01:55 PM | Comments (6)
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. 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. 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
Comments are listed in date ascending order (oldest first) | Post Comment
-
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
|
java.net RSS Feeds
