Hello,
I am part of a team maintaining the code for an EJB3-based application.
The code review showed that the developers made extensive use of [double-checked locking|http://www.javaworld.com/jw-02-2001/jw-0209-double.html] (DCL for short).
Although this is a bad smell, and certainly done as an oversight rather than a conscious decision, we are somewaht confident that this does not jeopardize the (current) thread-safety of the appliction.
I am opening this thread to verify this evaluation, and to collect arguments to have this changed.
The idea is that:
- if the idiom as such is thread-safe (I believe this to be the case), the version currently in production does not need a P1 patch!
- if the idiom is useless (I believe this to be the case), the next version should get rid of those idioms, be it for readability's sake.
- if the idiom is risky in the long run (I believe this to be the case as well), the next version must get rid of those.
_1) Is instance-level DCL thread-safe in a stateless session bean?_
The risk of DCL is that on certain hardware platforms, a thread might read an intermediate and inconsistent value of a variable from main memory, whereas another thread has modified the variable (but not committed it yet to main memory).
In particular, as DCL is often used to "optimize" lazy instantiations (see example below), but the runtime may turn out to perform the initialization sequence twice.
This is the case of the occurences I found (check an instance flag to avoid a costly instance init):
public Result instanceMethod(...) {
if (!initDone) {
synchronized (lock) {
if (!initDone) {
costlyInit();
initDone = true;
}
}
}
...
}
However, as this is in a Steless Session bean class (annotated with @Stateless), my understanding is that a given instance can only be used by one thread concurrently (i.e., the same instance will not be used by another thread until the EJB method has returned and the instance has been returned to the "method-ready" instances pool).
So in that particular situation, I think this DCL usage is safe (useless, but safe).
Am I correct?
In particular, am I guaranteed that the single-threadedness of the instance methods will involve synchronized block in the container's framework code, which, beside guarding against concurrent access to the same instance, also guarantee flushing of thread memory to the main memory?
_2) Is instance-level DCL useful in a stateless session bean?_
In my statement above, I claimed that this idiom was useless in this particular case.
I plan to have the costlyInit() moved to the constructor: it's not as if the instance was a rare component that might never be used and we would like to spare the costly init: the instance
will be used to serve client requests, and the init will be necessary. The only drawback is that if the container choose to instantiate 2K instances at startuip, we would do the costly inits upfront, before the client requests arrive - which is a point to be adressed at the container configuration level (tuning the pool's initial and growth properties).
Again, am I correct in my reasoning, and in the assumption that all major EJB containers enable to tune stateless bean pools'sizes (at least Glassfish, our current target, does)?
_3) Reasons to get rid of these DCL idioms:_
Would you agree that we'd better remove these idioms?
If so, I may need more psychological than technical support here, but maybe you can help me collect more arguments :o)
The point is, it may be difficult to convince the developers to remove these "currently harmless" idioms. My reasoning to get rid of them is along the lines of:
- if it's useless, remove it (YAGNI): it only obfuscates the code
- as such, they already contain an against-the-rules idiom: using a synchronized block within EJB code is contrary to the coding guidelines, although I can't find it stated in the EJB3 specs:
- - I remember reading and hearing this advice/rule, and it used to be strictly followed in my former server-side work.
- - Of course I found various postings article about this, such as
this and this but no link to an "official rule".
- - [This thread|http://forums.sun.com/thread.jspa?threadID=195720] suggest it was explicit in the EJB 2 specs.
- - But it's not mentioned in the EJB 3.0 spec; is it still forbidden to code low-level synchronization in a bean instance? And is there an official document as of EJB3 that states this explicitly?
- if the init code (including the DCL guard) is later refactored into helper classes, or is broken down across instance and static calls, it will be hard to track whether synchronization is appropriately performed. In particular the single-threaded access to helper objects is not guaranteed (and certainly a wrong assumption if helper instances are shared).
Anything I've forgotten/overlooked?
Edited by: jduprez on Sep 8, 2009 8:52 AM