Incorrect validation of DST in java.util.SimpleTimeZone

[David Holmes]

Hi Peter,

On 11/11/2017 8:06 PM, Peter Levart wrote:
> Hi Venkateswara R Chintala,
> 
> I would like to remind that TimeZone.clone() is also in the code path of 
> java.time.ZoneId.systemDefault() where it was relied on to be optimized 
> by JIT to never actually allocate the cloned TimeZone object by 
> employing escape analysis.

remind? Where is this documented?

And given:

     public static TimeZone getDefault() {
         return (TimeZone) getDefaultRef().clone();
     }

how can it not allocate??

David
-----

> It would be nice to verify if this patch 
> keeps that behavior. To test this, consider a JMH benchmark that simply 
> invokes ZoneId.systemDefault() and returns it from the test method. 1st 
> verify that unmodified code, when JITed, performs no allocations. Then 
> test with modified code by applying the patch and see if there's any 
> difference. Hint: use "-prof gc" command line options to run the 
> benchmarks.jar.
> 
> Regards, Peter
> 
> 
> On 11/11/17 06:53, Venkateswara R Chintala wrote:
>> Thanks Sean. I am pasting the patch here:
>>
>> --- old/src/java.base/share/classes/java/util/SimpleTimeZone.java 
>> 2017-11-11 11:17:38.643867420 +0530
>> +++ new/src/java.base/share/classes/java/util/SimpleTimeZone.java 
>> 2017-11-11 11:17:38.375870421 +0530
>> @@ -868,7 +868,11 @@
>>       */
>>      public Object clone()
>>      {
>> -        return super.clone();
>> +        // Invalidate the time zone cache while cloning as it
>> +        // can be inconsistent due to race condition.
>> +        SimpleTimeZone tz = (SimpleTimeZone) super.clone();
>> +        tz.invalidateCache();
>> +        return tz;
>>      }
>>
>>      /**
>> --- /dev/null    2017-11-02 17:09:59.155627814 +0530
>> +++ new/test/java/util/TimeZone/SimpleTimeZoneTest.java 2017-11-11 
>> 11:17:38.867864912 +0530
>> @@ -0,0 +1,55 @@
>> +/*
>> + * @test
>> + * @summary Tests the race condition between 
>> java.util.TimeZone.getDefault() and java.util.GregorianCalendar()
>> + * @run main SimpleTimeZoneTest
>> +*/
>> +
>> +import java.util.Calendar;
>> +import java.util.GregorianCalendar;
>> +import java.util.SimpleTimeZone;
>> +import java.util.TimeZone;
>> +
>> +public class SimpleTimeZoneTest extends Thread {
>> +    Calendar cal;
>> +
>> +    public SimpleTimeZoneTest (Calendar cal) {
>> +        this.cal = cal;
>> +    }
>> +
>> +    public static void main (String[] args) {
>> +        TimeZone stz = new SimpleTimeZone(7200000, "Asia/Jerusalem", 
>> Calendar.MARCH, 27, 0, 3600000, Calendar.SEPTEMBER, 16, 0, 3600000);
>> +        TimeZone.setDefault(stz);
>> +
>> +        SimpleTimeZoneTest stt = new SimpleTimeZoneTest(new 
>> GregorianCalendar());
>> +        stt.setDaemon(true);
>> +        stt.start();
>> +
>> +        for (int i = 0; i < 50000; i++) {
>> +            Calendar cal = new GregorianCalendar();
>> +            cal.clear();
>> +            cal.getTimeInMillis();
>> +            cal.set(2014, 2, 2);
>> +            cal.clear();
>> +            cal.getTimeInMillis();
>> +            cal.set(1970, 2, 2);
>> +        }
>> +
>> +    }
>> +
>> +    public void run() {
>> +        while (true) {
>> +            cal.setTimeZone(TimeZone.getDefault());
>> +            cal.clear();
>> +            cal.set(2008, 9, 9);
>> +            Calendar calInst = java.util.Calendar.getInstance();
>> +            calInst.setTimeInMillis(cal.getTimeInMillis());
>> +
>> +            if (calInst.get(java.util.Calendar.HOUR_OF_DAY) != 
>> cal.get(java.util.Calendar.HOUR_OF_DAY) ||
>> +                calInst.get(java.util.Calendar.MINUTE) != 
>> cal.get(java.util.Calendar.MINUTE) ||
>> +                calInst.get(java.util.Calendar.SECOND) != 
>> cal.get(java.util.Calendar.SECOND) ||
>> +                calInst.get(java.util.Calendar.MILLISECOND) != 
>> cal.get(java.util.Calendar.MILLISECOND)) {
>> +                    throw new RuntimeException("Test failed");
>> +            }
>> +        }
>> +    }
>> +}
>>
>>
>> On 10/11/17 9:29 PM, Seán Coffey wrote:
>>> I think the OpenJDK mailing lists accept attachments if in patch 
>>> format. If it's a simple short patch, it's acceptable to paste it 
>>> into email body.
>>>
>>> Easiest solution is to use webrev[1]. If you can't upload this to 
>>> cr.openjdk.java.net - then one of your colleagues may be able to help.
>>>
>>> [1] http://openjdk.java.net/guide/webrevHelp.html
>>>
>>> Regards,
>>> Sean.
>>>
>>> On 10/11/17 12:18, Venkateswara R Chintala wrote:
>>>> Looks like the patch attached earlier is not visible. As this is my 
>>>> first contribution, please let me know how I can send the patch for 
>>>> review.
>>>>
>>>>
>>>> On 10/11/17 5:37 PM, Venkateswara R Chintala wrote:
>>>>> Hi,
>>>>>
>>>>> In a multi-threaded environment, when java.util.SimpleTimeZone 
>>>>> object is used to create a default timezone, there can be a race 
>>>>> condition between the methods java.util.Timezone.getDefault() and 
>>>>> java.util.Timezone.getDefaultRef() which can result in 
>>>>> inconsistency of cache that is used to validate a particular 
>>>>> time/date in DST.
>>>>>
>>>>> When a thread is cloning a default timezone object (SimpleTimeZone) 
>>>>> and at the same time if a different thread modifies the time/year 
>>>>> values, then the cache values (cacheYear, cacheStart, cacheEnd) can 
>>>>> become inconsistent which leads to incorrect DST determination.
>>>>>
>>>>> We considered two approaches to fix the issue.
>>>>>
>>>>> 1)Synchronize access to cloning default timezone object when cache 
>>>>> is being modified.
>>>>>
>>>>> 2)Invalidate the cache while returning the clone.
>>>>>
>>>>> We preferred the second option as synchronization is more expensive.
>>>>>
>>>>> We have attached the patch and jtreg testcase. Please review.
>>>>>
>>>>
>>>
>>>
>>
>