8191216: SimpleTimeZone.clone() has a data race on cache fields
authorplevart
Tue, 12 Dec 2017 00:30:57 +0100
changeset 48239 8067e9cba973
parent 48238 9f225d4387e2
child 48240 e9ad230ea455
8191216: SimpleTimeZone.clone() has a data race on cache fields Reviewed-by: alanb, naoto
src/java.base/share/classes/java/util/SimpleTimeZone.java
test/jdk/java/util/TimeZone/SimpleTimeZoneCloneRaceTest.java
--- a/src/java.base/share/classes/java/util/SimpleTimeZone.java	Mon Dec 11 11:45:02 2017 -0800
+++ b/src/java.base/share/classes/java/util/SimpleTimeZone.java	Tue Dec 12 00:30:57 2017 +0100
@@ -548,12 +548,11 @@
 
       computeOffset:
         if (useDaylight) {
-            synchronized (this) {
-                if (cacheStart != 0) {
-                    if (date >= cacheStart && date < cacheEnd) {
-                        offset += dstSavings;
-                        break computeOffset;
-                    }
+            Cache cache = this.cache;
+            if (cache != null) {
+                if (date >= cache.start && date < cache.end) {
+                    offset += dstSavings;
+                    break computeOffset;
                 }
             }
             BaseCalendar cal = date >= GregorianCalendar.DEFAULT_GREGORIAN_CUTOVER ?
@@ -671,14 +670,13 @@
     }
 
     private int getOffset(BaseCalendar cal, BaseCalendar.Date cdate, int year, long time) {
-        synchronized (this) {
-            if (cacheStart != 0) {
-                if (time >= cacheStart && time < cacheEnd) {
-                    return rawOffset + dstSavings;
-                }
-                if (year == cacheYear) {
-                    return rawOffset;
-                }
+        Cache cache = this.cache;
+        if (cache != null) {
+            if (time >= cache.start && time < cache.end) {
+                return rawOffset + dstSavings;
+            }
+            if (year == cache.year) {
+                return rawOffset;
             }
         }
 
@@ -689,11 +687,7 @@
             if (time >= start && time < end) {
                 offset += dstSavings;
             }
-            synchronized (this) {
-                cacheYear = year;
-                cacheStart = start;
-                cacheEnd = end;
-            }
+            this.cache = new Cache(year, start, end);
         } else {
             if (time < end) {
                 // TODO: support Gregorian cutover. The previous year
@@ -711,12 +705,7 @@
                 }
             }
             if (start <= end) {
-                synchronized (this) {
-                    // The start and end transitions are in multiple years.
-                    cacheYear = (long) startYear - 1;
-                    cacheStart = start;
-                    cacheEnd = end;
-                }
+                this.cache = new Cache((long) startYear - 1, start, end);
             }
         }
         return offset;
@@ -876,7 +865,7 @@
      * Generates the hash code for the SimpleDateFormat object.
      * @return the hash code for this object
      */
-    public synchronized int hashCode()
+    public int hashCode()
     {
         return startMonth ^ startDay ^ startDayOfWeek ^ startTime ^
             endMonth ^ endDay ^ endDayOfWeek ^ endTime ^ rawOffset;
@@ -1201,19 +1190,27 @@
 
     /**
      * Cache values representing a single period of daylight saving
-     * time. When the cache values are valid, cacheStart is the start
-     * time (inclusive) of daylight saving time and cacheEnd is the
-     * end time (exclusive).
+     * time. Cache.start is the start time (inclusive) of daylight
+     * saving time and Cache.end is the end time (exclusive).
      *
-     * cacheYear has a year value if both cacheStart and cacheEnd are
-     * in the same year. cacheYear is set to startYear - 1 if
-     * cacheStart and cacheEnd are in different years. cacheStart is 0
-     * if the cache values are void. cacheYear is a long to support
-     * Integer.MIN_VALUE - 1 (JCK requirement).
+     * Cache.year has a year value if both Cache.start and Cache.end are
+     * in the same year. Cache.year is set to startYear - 1 if
+     * Cache.start and Cache.end are in different years.
+     * Cache.year is a long to support Integer.MIN_VALUE - 1 (JCK requirement).
      */
-    private transient long cacheYear;
-    private transient long cacheStart;
-    private transient long cacheEnd;
+    private static final class Cache {
+        final long year;
+        final long start;
+        final long end;
+
+        Cache(long year, long start, long end) {
+            this.year = year;
+            this.start = start;
+            this.end = end;
+        }
+    }
+
+    private transient volatile Cache cache;
 
     /**
      * Constants specifying values of startMode and endMode.
@@ -1282,9 +1279,8 @@
     // Maximum number of rules.
     private static final int MAX_RULE_NUM = 6;
 
-    private synchronized void invalidateCache() {
-        cacheYear = startYear - 1;
-        cacheStart = cacheEnd = 0;
+    private void invalidateCache() {
+        cache = null;
     }
 
     //----------------------------------------------------------------------
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/java/util/TimeZone/SimpleTimeZoneCloneRaceTest.java	Tue Dec 12 00:30:57 2017 +0100
@@ -0,0 +1,137 @@
+/*
+ * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+import java.util.Calendar;
+import java.util.Locale;
+import java.util.SimpleTimeZone;
+import java.util.TimeZone;
+import java.util.function.Supplier;
+
+/*
+ * @test
+ * @bug 8191216
+ * @summary test that provokes race between cloning and lazily initializing
+ *          SimpleTimeZone cache fields
+ */
+public class SimpleTimeZoneCloneRaceTest {
+
+    public static void main(String[] args) throws InterruptedException {
+
+        // shared TZ user repeatedly samples sharedTZ and calculates offset
+        // using the shared instance
+        TimeZoneUser sharedTZuser = new TimeZoneUser(() -> sharedTZ);
+
+        // cloned TZ user repeatedly samples sharedTZ then clones it and
+        // calculates offset using the clone...
+        TimeZoneUser clonedTZuser = new TimeZoneUser(() -> {
+            // sample shared TZ
+            TimeZone tz = sharedTZ;
+            // do some computation that takes roughly the same time as it takes
+            // sharedTZUser to start changing cache fields in shared TZ
+            cpuHogTZ.getOffset(time);
+            // now clone the sampled TZ and return it, hoping the clone is done
+            // at about the right time....
+            return (TimeZone) tz.clone();
+        });
+
+        // start threads
+        Thread t1 = new Thread(sharedTZuser);
+        Thread t2 = new Thread(clonedTZuser);
+        t1.start();
+        t2.start();
+
+        // plant new SimpleTimeZone instances for 2 seconds
+        long t0 = System.currentTimeMillis();
+        do {
+            TimeZone tz1 = createSTZ();
+            TimeZone tz2 = createSTZ();
+            cpuHogTZ = tz1;
+            sharedTZ = tz2;
+        } while (System.currentTimeMillis() - t0 < 2000L);
+
+        sharedTZuser.stop = true;
+        clonedTZuser.stop = true;
+        t1.join();
+        t2.join();
+
+        System.out.println(
+            String.format("shared TZ: %d correct, %d incorrect calculations",
+                          sharedTZuser.correctCount, sharedTZuser.incorrectCount)
+        );
+        System.out.println(
+            String.format("cloned TZ: %d correct, %d incorrect calculations",
+                          clonedTZuser.correctCount, clonedTZuser.incorrectCount)
+        );
+        if (clonedTZuser.incorrectCount > 0) {
+            throw new RuntimeException(clonedTZuser.incorrectCount +
+                                       " fatal data races detected");
+        }
+    }
+
+    static SimpleTimeZone createSTZ() {
+        return new SimpleTimeZone(-28800000,
+                                  "America/Los_Angeles",
+                                  Calendar.APRIL, 1, -Calendar.SUNDAY,
+                                  7200000,
+                                  Calendar.OCTOBER, -1, Calendar.SUNDAY,
+                                  7200000,
+                                  3600000);
+    }
+
+    static volatile TimeZone cpuHogTZ = createSTZ();
+    static volatile TimeZone sharedTZ = createSTZ();
+    static final long time;
+    static final long correctOffset;
+
+    static {
+        TimeZone tz = createSTZ();
+        Calendar cal = Calendar.getInstance(tz, Locale.ROOT);
+        cal.set(2000, Calendar.MAY, 1, 0, 0, 0);
+        time = cal.getTimeInMillis();
+        correctOffset = tz.getOffset(time);
+    }
+
+    static class TimeZoneUser implements Runnable {
+        private final Supplier<? extends TimeZone> tzSupplier;
+
+        TimeZoneUser(Supplier<? extends TimeZone> tzSupplier) {
+            this.tzSupplier = tzSupplier;
+        }
+
+        volatile boolean stop;
+        int correctCount, incorrectCount;
+
+        @Override
+        public void run() {
+            while (!stop) {
+                TimeZone tz = tzSupplier.get();
+                int offset = tz.getOffset(time);
+                if (offset == correctOffset) {
+                    correctCount++;
+                } else {
+                    incorrectCount++;
+                }
+            }
+        }
+    }
+}