On 03/19/2015 04:46 PM, Peter Levart wrote:
On 03/19/2015 02:53 PM, Kumar Srinivasan wrote:
Mikhail,
You can move the common utilitieschangeDefaultTimeZoneToUtc and
restoreDefaultTimeZoneto Utils.java.
Also I am not sure how effective the test is ? does it catch the
issue ?
if you don't have the fix in PackerImpl and UnpackerImpl ?
Otherwise it looks good, and I can sponsor this patch for you.
Kumar
Hi Mikhail,
Is this code part of some utility so that it is executed exclusively
in it's own JVM, or can it also be executed by some public API or
internal JVM thread? It appears to be the later (used by
java.util.jar.Pack200 public API), but haven't checked.
In case of the later, I think it is very strange that the code changes
global JVM timezone, albeit just temporarily. This could affect other
code that needs default TZ and executes concurrently.
Even in case of the former, if the PackerImpl.pack can be executed by
multiple concurrent threads and if UnpackerImpl.unpack can be executed
by multiple concurrent threads, what guarantees that some
PackerImpl.pack thread is not executed concurrently with some
UnpackerImpl.unpack thread? You only track each method separately.
Regards, Peter
I guess this default TZ manipulation is needed because
JarOutputStream/JarInputStream don't provide constructors that would
take a TZ object, but always use default TZ, right?
The cleanest way would be to add such constructors, but if this is too
much for 8u, then perhaps some internal ThreadLocal<TZ> could be exposed
to both PackerImpl/UnpackerImpl and Jar[Zip]Input[Output] stream
internals. It think the use of default TZ in Jar/Zip/Streams is very
localized. It is needed to convert local time (specified as DOS time) to
and from epoch-based time exposed in ZipEntry. The code is located in
package-private class java.util.zip.ZipUtils in methods dosToJavaTime
/javaToDosTime (or equivalent extendedDosToJavaTime /
javaToExtendedDosTime in JDK9).
Another way would be to provide a general thread-local default TZ
override in TimeZone itself. Something like the following:
===================================================================
--- jdk/src/share/classes/java/util/TimeZone.java
+++ jdk/src/share/classes/java/util/TimeZone.java
@@ -630,8 +630,11 @@
* method doesn't create a clone.
*/
static TimeZone getDefaultRef() {
- TimeZone defaultZone = defaultTimeZone;
+ TimeZone defaultZone = threadLocalTimeZone.get();
if (defaultZone == null) {
+ defaultZone = defaultTimeZone;
+ }
+ if (defaultZone == null) {
// Need to initialize the default time zone.
defaultZone = setDefaultZone();
assert defaultZone != null;
@@ -713,6 +716,20 @@
defaultTimeZone = zone;
}
+ public void withThreadDefaultDo(Runnable runnable) {
+ TimeZone prevZone = threadLocalTimeZone.get();
+ threadLocalTimeZone.set(this);
+ try {
+ runnable.run();
+ } finally {
+ if (prevZone == null) {
+ threadLocalTimeZone.remove();
+ } else {
+ threadLocalTimeZone.set(prevZone);
+ }
+ }
+ }
+
/**
* Returns true if this zone has the same rule and offset as
another zone.
* That is, if this zone differs only in ID, if at all. Returns false
@@ -760,6 +777,8 @@
*/
private String ID;
private static volatile TimeZone defaultTimeZone;
+ private static final ThreadLocal<TimeZone> threadLocalTimeZone =
+ new ThreadLocal<>();
static final String GMT_ID = "GMT";
private static final int GMT_ID_LENGTH = 3;
Regards, Peter
On 3/18/2015 3:07 PM, Kumar Srinivasan wrote:
Hello Mikhail,
Hi all,
Bug: https://bugs.openjdk.java.net/browse/JDK-8066985
The problem is that packer/unpacker changes global state( default
time zone ) without proper synchronization:
http://hg.openjdk.java.net/jdk9/client/jdk/file/a159e5358e25/src/java.base/share/classes/com/sun/java/util/jar/pack/PackerImpl.java#l85
however javadoc says that it's save to use it in concurrent way if
each thread has it's own packer/unpacker instance:
http://hg.openjdk.java.net/jdk9/client/jdk/file/a159e5358e25/src/java.base/share/classes/java/util/jar/Pack200.java#l149
The fix is:
1. first packer/unpacker saves default time zone
2. the last set it back.
Webrev:
http://cr.openjdk.java.net/~mcherkas/8066985/webrev.00/
The above link is wrong!, it takes me to webrev for 8073187,
which has only the PackerImpl changes.
I am guessing the link should be:
http://cr.openjdk.java.net/~mcherkas/8066985/webrev.00/
Kumar
Thanks,
Mikhail.
