Hi David,
On 11/14/2017 10:28 PM, David Holmes wrote:
Hi Peter,
On 15/11/2017 1:02 AM, Peter Levart wrote:
Hi David,
On 11/11/2017 07:51 AM, David Holmes wrote:
AFAICS SimpleTimeZone is simply not thread-safe. It has state that
can be modified concurrently without synchronization and with fields
not even declared volatile. Only the "cache" makes an attempt to use
synchronization. So clone() is never guaranteed to actually produce
a copy with valid/consistent field values.
The suggested patch certainly improves the situation by at least
resetting the cache of the cloned instance before returning it.
The instance of SimpleTimeZone that is shared among threads
(internally in JDK) is the defaultTimeZone instance (obtained through
package-private TimeZone.getDefaultRef() method). I checked the
usages and they seem to be "read-only" - not modifying the instance,
just obtaining information from it. The cache OTOH, as you say, is
synchronized.
The initial problem statement was:
"When a thread is cloning a default timezone object (SimpleTimeZone)
and at the same time if a different thread modifies the time/year
values, ..."
so that doesn't seem to be read-only. Though perhaps it is a very
specific race.
User code may do that with its own instances, but that would be invalid
usage. There is no evidence (at least I haven't spotted it yet) that JDK
code does the same too. As far as I have checked, internal JDK code is
aware of the fact that defaultTimeZone instance is a shared instance.
For example, take a protected java.util.Calendar no-arg constructor. It
initializes the Calendar instance with the result of
TimeZone.getDefaultRef() which returns a shared instance. But it also
sets Calendar's 'sharedZone' flag, marking that the TimeZone instance it
references is a shared instance. Methods that would expose such instance
to user code are careful not to do that. For example:
public TimeZone getTimeZone()
{
// If the TimeZone object is shared by other Calendar
instances, then
// create a clone.
if (sharedZone) {
zone = (TimeZone) zone.clone();
sharedZone = false;
}
return zone;
}
(BTW, this method may expose shared instance to user code if it is
invoked concurrently from multiple threads on the same Calendar instance
- there's no attempt to prevent writes to zone and sharedZone fields to
be observed in non-program order by some concurrent thread)
So I believe the situation is not so critical as it seemed at first.
There may be other concurrency bugs like in above getTimeZone() method
lurking, but the real problem here is cache* fields that may be modified
while using "read-only" part of API. All such accesses are synchronized,
except in the Object.clone() which reads them without holding the lock.
TimeZone and subclasses seem to be designed to be modified by single
thread only, but can be used from multiple threads to read the
information from them, including lazily computed and cached
information. Usage withing JDK seems to comply with that.
There's certainly no documentation of any such intent, or design.
Seems more like the synchronization has been added (or not) based on
how it is used within JDK rather than considering the actual API of
the public types.
Mercurial history does not go that far in the past to be able to see if
synchronization for cache* fields was added at some point and why. My
conclusions are based solely on the state of current code.
Regards, Peter
Venkat's patch therefore correctly fixes the remaining issue that is
Okay. As I said it certainly makes things better.
Cheers,
David
observed when the shared SimpleTimeZone instance is being cloned
while also being accessed from multiple threads in read-only mode.
Invalidating cache of the cloned instance just before returning it
from clone() method means that instance obtained from
TimeZone.getDefault() will never get cached state from original
instance and will always have to re-compute it, but I think this is
still better than synchronizing on the original instance which may
never be optimized away (i.e. elided) by JIT.
Regards, Peter
David
On 11/11/2017 3:53 PM, 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.
