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.
4 comments:
Tim,
I really appreciate your insights on concurrency. I never fully grokked the downsides of setter injection before your explanation. I wish more open source projects recieved this type of scrutiny.
The Restlet team seems very receptive to constructive criticism, thus improving the feedback loop. I look forward to seeing changes based on your findings in the next version.
Please keep up the great work.
(edit) Followup: Restlet 1.1M1 milestone release includes some concurrency fixes ... but further work remains to be done!
Hi Tim.
I don't work with beans, Restlet or Spring. But I support your using volatile even though your fields are setter-injected only before publication. Otherwise JMM doesn't guarantee that a consumer thread will see values up-to-date, unless there was a synchronization action between the producer and all consumer threads.
Sadly, not even Sun's J2SE sources follow this. The problem is in current JMM and false confidence it gave people. Effectively final fields - ones that are never modified to other threads - should be guaranteed thread-safe.
Extra benefits: effectively immutable arrays, checked in runtime! Read more at http://negev.wordpress.com/java-memory-brief.
Post a Comment