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. 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.
