r998053 class org/apache/tomcat/util/http/FastHttpDateFormat http://svn.apache.org/repos/asf/tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/http/FastHttpDateFormat.java There is double-check anti-pattern in public static final String getCurrentDate() method on protected static long currentDateGenerated variable Concurrent read on line 112 Concurrent write on line 115 P.S.: Maybe I have mistake with determine component
The currentDateGenerated field is read outside the synch. block, and is not volatile. So if one thread calls getCurrentDate() and updates the field, another thread calling getCurrentDate() may see a stale value. Now a stale value will be smaller than the proper value, so the worst that can happen is that the synch. block is invoked unnecessarily. The synch. block will pick up the true value, and skip the format call if necessary. However, this assumes that the field is only updated by the getCurrentDate() method. As the field is not private, that is not guranteed. Seems to me that the field should be made private, and a comment added to explain why the double-check is OK in this case. If read-only access is needed, a getter could be added for the binary field.
It looks scarier than that. In short, see http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html Case1: Thread1 executes: long now = System.currentTimeMillis(); if ((now - currentDateGenerated) > 1000) { synchronized (format) { if ((now - currentDateGenerated) > 1000) { currentDateGenerated = now; Thread2 executes: long now = System.currentTimeMillis(); if ((now - currentDateGenerated) > 1000) { Thread2 skips the "if" part. If these were the first calls to this function the return value in Thread2 will be null. ----- Case2: Thread1 executes: currentDate = format.format(new Date(now)); Thread2 goes through fast path and executes return currentDate; The object currentDate was not properly published and according to the Java memory model (if I understand it correctly) may be incomplete. Of course, in practice you will not see any problem in 99.9% of the cases.
(In reply to comment #2) > It looks scarier than that. > In short, see > http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html Not all double-checked locking is evil. The simple fix is to just reverse these two lines: currentDateGenerated = now; currentDate = format.format(new Date(now)); By setting currentDate prior to currentDateGenerated, the potential of returning null is eliminated, and unnecessary locking is still avoided. (This is in addition to making the field private, as Sebb noted.) Case 2 is not a problem; the Java language spec requires operations to be visible to other threads in program order, so the all operations for building the new formatted String must be complete before the reference to it is stored in the field. - Chuck
>> The simple fix is to just reverse these two lines: I afraid, it won't work. The compiler is free to reverse these lines back. (maybe not in this particular case, but certainly in general). >> the Java language spec requires operations to be visible to other threads in program order Does it? (I am a C++ guy, could you point me to the place in Java docs where this is stated?) From what I can see, the order is guaranteed only in the presence of happens-before relation (volatile write followed by volatile read or a mutex unlock followed by a mutex lock).
Also, there are problem with 64 bit variable currentDateGenerated http://java.sun.com/docs/books/jls/third_edition/html/memory.html quote: For the purposes of the Java programming language memory model, a single write to a non-volatile long or double value is treated as two separate writes: one to each 32-bit half. This can result in a situation where a thread sees the first 32 bits of a 64 bit value from one write, and the second 32 bits from another write. Writes and reads of volatile long and double values are always atomic. Writes to and reads of references are always atomic, regardless of whether they are implemented as 32 or 64 bit values. So in first (lock-free) check if ((now - currentDateGenerated) > 1000) We can read inconsistante state
(In reply to comment #4) > > The simple fix is to just reverse these two lines: > I afraid, it won't work. > The compiler is free to reverse these lines back. Not in this case, due to the possibility of the new operator throwing an exception and the constraint specified in section 11.3.1: "Exceptions are precise: when the transfer of control takes place, all effects of the statements executed and expressions evaluated before the point from which the exception is thrown must appear to have taken place. No expressions, statements, or parts thereof that occur after the point from which the exception is thrown may appear to have been evaluated." If there were no possibility of an exception, your statement would be true. > > the Java language spec requires operations to be > > visible to other threads in program order > Does it? That's my interpretation of this statement in section 17.4: "Each time the evaluation of thread t generates an inter-thread action, it must match the inter-thread action a of t that comes next in program order." All reads and writes of shared variables are inter-thread actions. - Chuck
(In reply to comment #5) > Also, there are problem with 64 bit variable currentDateGenerated This is a problem (albeit a fairly minor one); technically, the field should be marked volatile to insure all 64 bits are read and written together. Not sure what that does to code generation on a 32-bit machine, but I suspect it's not pretty. Another reason we should all be running 64-bit boxes... - Chuck
>> If there were no possibility of an exception, your statement would be true. I am not sure about this particular case, but in general a compiler (JIT) may statically figure out that an exception will never be thrown and eliminate the exception handling code completely -- and then shuffle the code in arbitrary way. I don't think that 'this code may throw exception' may be used as synchronization.
Fixed in trunk and proposed for 6.0.x
Fixed in 6.0.x for 6.0.30 onwards.