Bug 49972 - Double-check idiom. Possible data-race.
Summary: Double-check idiom. Possible data-race.
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 6
Classification: Unclassified
Component: Manager application (show other bugs)
Version: 6.0.29
Hardware: PC All
: P2 normal (vote)
Target Milestone: default
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-22 04:44 UTC by Sergey Vorobyev
Modified: 2010-10-13 11:07 UTC (History)
2 users (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sergey Vorobyev 2010-09-22 04:44:02 UTC
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
Comment 1 Sebb 2010-09-22 06:53:24 UTC
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.
Comment 2 Kostya Serebryany 2010-09-22 09:56:49 UTC
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.
Comment 3 Chuck Caldarale 2010-09-22 10:20:36 UTC
(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
Comment 4 Kostya Serebryany 2010-09-22 10:53:58 UTC
>> 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).
Comment 5 Sergey Vorobyev 2010-09-22 11:46:36 UTC
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
Comment 6 Chuck Caldarale 2010-09-22 12:53:46 UTC
(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
Comment 7 Chuck Caldarale 2010-09-22 12:58:53 UTC
(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
Comment 8 Kostya Serebryany 2010-09-23 04:03:35 UTC
>> 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.
Comment 9 Mark Thomas 2010-10-07 10:07:22 UTC
Fixed in trunk and proposed for 6.0.x
Comment 10 Mark Thomas 2010-10-13 11:07:03 UTC
Fixed in 6.0.x for 6.0.30 onwards.