Code review request for 6998871 Support making the Throwable.stackTrace field immutable
Hello. Returning to some earlier work, I've developed a proposed fix for 6998871 Support making the Throwable.stackTrace field immutable http://cr.openjdk.java.net/~darcy/6998871.2/ One constructor of Throwable now takes an additional boolean argument to make the stack trace information immutable. Analogous constructors are added to Exception, RuntimeException, and Error. Mandy and David have already reviewed the change; I'm interested in getting additional feedback on the design of the API. Cheers, -Joe
RE: Code review request for 6998871 Support making the Throwable.stackTrace field immutable
Just wonder what is the purpose of dummy parameter in native fillInStackTrace method. Couldn't we simply rename it (e.g. to fillInStackTrace0) Date: Wed, 6 Apr 2011 23:29:02 -0700 From: joe.da...@oracle.com To: core-libs-dev@openjdk.java.net Subject: Code review request for 6998871 Support making the Throwable.stackTrace field immutable Hello. Returning to some earlier work, I've developed a proposed fix for 6998871 Support making the Throwable.stackTrace field immutable http://cr.openjdk.java.net/~darcy/6998871.2/ One constructor of Throwable now takes an additional boolean argument to make the stack trace information immutable. Analogous constructors are added to Exception, RuntimeException, and Error. Mandy and David have already reviewed the change; I'm interested in getting additional feedback on the design of the API. Cheers, -Joe
Re: Code review request for 6998871 Support making the Throwable.stackTrace field immutable
Dmytro Sheyko said the following on 04/07/11 17:50: Just wonder what is the purpose of dummy parameter in native fillInStackTrace method. Couldn't we simply rename it (e.g. to fillInStackTrace0) Using an overload instead of renaming the native method was initially done to avoid having to make changes to the stacktrace filtering code in the VM. It turns out that we need to make a change anyway, but it is easier if we stick with the one name for the method to be filtered out. This isn't set in concrete though. David Date: Wed, 6 Apr 2011 23:29:02 -0700 From: joe.da...@oracle.com To: core-libs-dev@openjdk.java.net Subject: Code review request for 6998871 Support making the Throwable.stackTrace field immutable Hello. Returning to some earlier work, I've developed a proposed fix for 6998871 Support making the Throwable.stackTrace field immutable http://cr.openjdk.java.net/~darcy/6998871.2/ One constructor of Throwable now takes an additional boolean argument to make the stack trace information immutable. Analogous constructors are added to Exception, RuntimeException, and Error. Mandy and David have already reviewed the change; I'm interested in getting additional feedback on the design of the API. Cheers, -Joe
Re: How can I get all emails from this mailing list
On 3/2/11 9:55 AM, Charles Lee wrote: Is there any place, which is like http://markmail.org/, holding all the mailing from openjdk mailing list? http://markmail.org/search/?q=list%3Anet.java.openjdk cheers, dalibor topic -- Oracle http://www.oracle.com Dalibor Topic | Java F/OSS Ambassador Phone: +494023646738 tel:+494023646738 | | | Mobile: +491772664192 tel:+491772664192 Oracle Java Platform Group ORACLE Deutschland B.V. Co. KG | Nagelsweg 55 | 20097 Hamburg ORACLE Deutschland B.V. Co. KG Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Rijnzathe 6, 3454PV De Meern, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Jürgen Kunz, Marcel van de Molen, Alexander van der Ven Green Oracle http://www.oracle.com/commitment Oracle is committed to developing practices and products that help protect the environment
Re: Need reviewers: Update of jaxp 1.4.5 source drop bundle
On 3/3/11 3:01 AM, Dr Andrew John Hughes wrote: How do we know what the actual changes are between these tarballs? Is there some JAXP repository somewhere these are derived from, with appropriate tagging? Afaik, the JAXP API and implementation are supplied by the upstream JAXP developers in the GlassFish JAXP project, i.e. http://jaxp.java.net/ . The source code repository is at http://java.net/projects/jaxp-sources/sources/svn/show cheers, dalibor topic -- Oracle http://www.oracle.com Dalibor Topic | Java F/OSS Ambassador Phone: +494023646738 tel:+494023646738 | | | Mobile: +491772664192 tel:+491772664192 Oracle Java Platform Group ORACLE Deutschland B.V. Co. KG | Nagelsweg 55 | 20097 Hamburg ORACLE Deutschland B.V. Co. KG Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Rijnzathe 6, 3454PV De Meern, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Jürgen Kunz, Marcel van de Molen, Alexander van der Ven Green Oracle http://www.oracle.com/commitment Oracle is committed to developing practices and products that help protect the environment
RE: Code review request for 6998871 Support making the Throwable.stackTrace field immutable
Thanks. Got it. Date: Thu, 7 Apr 2011 18:03:42 +1000 From: david.hol...@oracle.com To: dmytro_she...@hotmail.com CC: joe.da...@oracle.com; core-libs-dev@openjdk.java.net Subject: Re: Code review request for 6998871 Support making the Throwable.stackTrace field immutable Dmytro Sheyko said the following on 04/07/11 17:50: Just wonder what is the purpose of dummy parameter in native fillInStackTrace method. Couldn't we simply rename it (e.g. to fillInStackTrace0) Using an overload instead of renaming the native method was initially done to avoid having to make changes to the stacktrace filtering code in the VM. It turns out that we need to make a change anyway, but it is easier if we stick with the one name for the method to be filtered out. This isn't set in concrete though. David
Re: Suspected regression: fix for 6735255 causes delay in GC of ZipFile InputStreams, increase in heap demand
On Mon, 2011-04-04 at 09:04 +1000, David Holmes wrote:
1. If a call to close() occurs around the same time as finalization
occurs then the finalizer thread will set inFinalizer to true, at which
point the thread executing close() can see it is true and so may skip
the releaseInflater(inf) call.
2. As isClosed is not volatile, and available() is not synchronized, a
thread calling available() on a closed stream may not see that it has
been closed and so will likely encounter an IOException rather than
getting a zero return.
Even if #1 is not a practical problem I'd be inclined to make the
finalizer synchronized as well. By doing that you're effectively
ensuring that premature finalization of the stream won't happen.
I tend to agree, especially as it also makes the intention of the code
clearer.
For #2 it is a tricky call. If you don't actually expect the stream
object to be used by multiple threads then using a synchronized block to
read isClosed will be cheap in VMs that go to great effort to reduce the
cost of unnecessary synchronization (eg biased-locking in hotspot).
Otherwise making isClosed volatile is likely the better option.
The check at the start of available() guards the logic beyond (which
uses a value from the inflater object, which would not be valid if the
stream has been closed()).
Because of this, I think it would be clearer to synchronize the
available() method too.
As you say, it is extremely likely that, in practice, this
synchronization will never be contended, and so won't impact performance
significantly (in modern VMs).
Please find below an updated changeset with these modifications,
Thanks for your advice in this,
Neil
--
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
# HG changeset patch
# User Neil Richards neil.richa...@ngmr.net, neil_richa...@uk.ibm.com
# Date 1300289208 0
# Branch zip-heap
# Node ID db52003c128c8706f45e0b2bf9f98d5e905d2090
# Parent 554adcfb615e63e62af530b1c10fcf7813a75b26
7031076: Retained ZipFile InputStreams increase heap demand
Summary: Allow unreferenced ZipFile InputStreams to be finalized, GC'd
Contributed-by: neil.richa...@ngmr.net
diff -r 554adcfb615e -r db52003c128c
src/share/classes/java/util/zip/ZipFile.java
--- a/src/share/classes/java/util/zip/ZipFile.java Wed Mar 16 15:01:07
2011 -0700
+++ b/src/share/classes/java/util/zip/ZipFile.java Wed Mar 16 15:26:48
2011 +
@@ -30,11 +30,14 @@
import java.io.IOException;
import java.io.EOFException;
import java.io.File;
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.WeakReference;
import java.nio.charset.Charset;
import java.util.Vector;
import java.util.Enumeration;
import java.util.Set;
import java.util.HashSet;
+import java.util.Iterator;
import java.util.NoSuchElementException;
import java.security.AccessController;
import sun.security.action.GetPropertyAction;
@@ -315,7 +318,16 @@
private static native void freeEntry(long jzfile, long jzentry);
// the outstanding inputstreams that need to be closed.
-private SetInputStream streams = new HashSet();
+private final SetWeakReferenceInputStream streams = new HashSet();
+private final ReferenceQueue? super InputStream staleStreamQueue =
+new ReferenceQueue();
+
+private void clearStaleStreams() {
+Object staleStream;
+while (null != (staleStream = staleStreamQueue.poll())) {
+streams.remove(staleStream);
+}
+}
/**
* Returns an input stream for reading the contents of the specified
@@ -339,6 +351,7 @@
ZipFileInputStream in = null;
synchronized (this) {
ensureOpen();
+clearStaleStreams();
if (!zc.isUTF8() (entry.flag EFS) != 0) {
jzentry = getEntry(jzfile, zc.getBytesUTF8(entry.name), false);
} else {
@@ -351,51 +364,19 @@
switch (getEntryMethod(jzentry)) {
case STORED:
-streams.add(in);
+streams.add(
+new WeakReferenceInputStream(in, staleStreamQueue));
return in;
case DEFLATED:
-final ZipFileInputStream zfin = in;
// MORE: Compute good size for inflater stream:
long size = getEntrySize(jzentry) + 2; // Inflater likes a bit
of slack
if (size 65536) size = 8192;
if (size = 0) size = 4096;
-InputStream is = new InflaterInputStream(zfin, getInflater(),
(int)size) {
-private boolean isClosed = false;
-
-public void close() throws IOException {
-if (!isClosed) {
-super.close();
-releaseInflater(inf);
-
Re: Need reviewers: Update of jaxp 1.4.5 source drop bundle
On 07/04/2011, Dalibor Topic dalibor.to...@oracle.com wrote: On 3/3/11 3:01 AM, Dr Andrew John Hughes wrote: How do we know what the actual changes are between these tarballs? Is there some JAXP repository somewhere these are derived from, with appropriate tagging? Afaik, the JAXP API and implementation are supplied by the upstream JAXP developers in the GlassFish JAXP project, i.e. http://jaxp.java.net/ . The source code repository is at http://java.net/projects/jaxp-sources/sources/svn/show http://java.net/projects/jaxp-sources/sources/svn/show/tags shows no tags in the last six months. What does this tarball correspond to? cheers, dalibor topic -- -- Andrew :-) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) Support Free Java! Contribute to GNU Classpath and the OpenJDK http://www.gnu.org/software/classpath http://openjdk.java.net PGP Key: 94EFD9D8 (http://subkeys.pgp.net) Fingerprint: F8EF F1EA 401E 2E60 15FA 7927 142C 2591 94EF D9D8
Review request for removal of lint warnings in DriverManager, CR 7034656
Hi all, This is a request for a review of the changes to remove the lint warnings for DriverManager. The diff is at http://cr.openjdk.java.net/~lancea/7034656/. Thank you. Regards, Lance Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: Need reviewers: Update of jaxp 1.4.5 source drop bundle
On 4/7/11 3:49 PM, Dr Andrew John Hughes wrote: On 07/04/2011, Dalibor Topic dalibor.to...@oracle.com wrote: On 3/3/11 3:01 AM, Dr Andrew John Hughes wrote: How do we know what the actual changes are between these tarballs? Is there some JAXP repository somewhere these are derived from, with appropriate tagging? Afaik, the JAXP API and implementation are supplied by the upstream JAXP developers in the GlassFish JAXP project, i.e. http://jaxp.java.net/ . The source code repository is at http://java.net/projects/jaxp-sources/sources/svn/show http://java.net/projects/jaxp-sources/sources/svn/show/tags shows no tags in the last six months. What does this tarball correspond to? Good point. CC:ing Joe. cheers, dalibor topic -- Oracle http://www.oracle.com Dalibor Topic | Java F/OSS Ambassador Phone: +494023646738 tel:+494023646738 | | | Mobile: +491772664192 tel:+491772664192 Oracle Java Platform Group ORACLE Deutschland B.V. Co. KG | Nagelsweg 55 | 20097 Hamburg ORACLE Deutschland B.V. Co. KG Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Rijnzathe 6, 3454PV De Meern, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Jürgen Kunz, Marcel van de Molen, Alexander van der Ven Green Oracle http://www.oracle.com/commitment Oracle is committed to developing practices and products that help protect the environment
Re: Review request for removal of lint warnings in DriverManager, CR 7034656
Lance Andersen - Oracle wrote: Hi all, This is a request for a review of the changes to remove the lint warnings for DriverManager. The diff is at http://cr.openjdk.java.net/~lancea/7034656/. Looks okay to me. -Alan
Re: Code review request for 6998871 Support making the Throwable.stackTrace field immutable
On 04/07/2011 08:29 AM, Joe Darcy wrote: Hello. Returning to some earlier work, I've developed a proposed fix for 6998871 Support making the Throwable.stackTrace field immutable http://cr.openjdk.java.net/~darcy/6998871.2/ One constructor of Throwable now takes an additional boolean argument to make the stack trace information immutable. Analogous constructors are added to Exception, RuntimeException, and Error. Mandy and David have already reviewed the change; I'm interested in getting additional feedback on the design of the API. Cheers, -Joe Joe, I don't think you need the sentinel in the serialized form, you have only two states: an immutable stacktrace (stacktrace == null) or a stacktrace. I think it's better to don't serialize the field stacktrace if the stacktrace is immutable. Also, FILLED_IN_STACK is not necessary, you can use EMPTY_STACKinstead, or if you find it hard to understand, at least FILLED_IN_STACK should be an empty array. Rémi
Re: Review request for removal of lint warnings in DriverManager, CR 7034656
On 04/07/2011 04:12 PM, Alan Bateman wrote: Lance Andersen - Oracle wrote: Hi all, This is a request for a review of the changes to remove the lint warnings for DriverManager. The diff is at http://cr.openjdk.java.net/~lancea/7034656/. Looks okay to me. -Alan So am I. Rémi
hg: jdk7/tl/jdk: 7034657: Update Creative Commons license URL in legal notices
Changeset: 31619dfa6a4a Author:dl Date: 2011-04-07 15:06 +0100 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/31619dfa6a4a 7034657: Update Creative Commons license URL in legal notices Reviewed-by: chegar ! src/share/classes/java/util/AbstractQueue.java ! src/share/classes/java/util/ArrayDeque.java ! src/share/classes/java/util/Deque.java ! src/share/classes/java/util/NavigableMap.java ! src/share/classes/java/util/NavigableSet.java ! src/share/classes/java/util/Queue.java ! src/share/classes/java/util/concurrent/AbstractExecutorService.java ! src/share/classes/java/util/concurrent/ArrayBlockingQueue.java ! src/share/classes/java/util/concurrent/BlockingDeque.java ! src/share/classes/java/util/concurrent/BlockingQueue.java ! src/share/classes/java/util/concurrent/BrokenBarrierException.java ! src/share/classes/java/util/concurrent/Callable.java ! src/share/classes/java/util/concurrent/CancellationException.java ! src/share/classes/java/util/concurrent/CompletionService.java ! src/share/classes/java/util/concurrent/ConcurrentHashMap.java ! src/share/classes/java/util/concurrent/ConcurrentLinkedDeque.java ! src/share/classes/java/util/concurrent/ConcurrentLinkedQueue.java ! src/share/classes/java/util/concurrent/ConcurrentMap.java ! src/share/classes/java/util/concurrent/ConcurrentNavigableMap.java ! src/share/classes/java/util/concurrent/ConcurrentSkipListMap.java ! src/share/classes/java/util/concurrent/ConcurrentSkipListSet.java ! src/share/classes/java/util/concurrent/CopyOnWriteArraySet.java ! src/share/classes/java/util/concurrent/CountDownLatch.java ! src/share/classes/java/util/concurrent/CyclicBarrier.java ! src/share/classes/java/util/concurrent/DelayQueue.java ! src/share/classes/java/util/concurrent/Delayed.java ! src/share/classes/java/util/concurrent/Exchanger.java ! src/share/classes/java/util/concurrent/ExecutionException.java ! src/share/classes/java/util/concurrent/Executor.java ! src/share/classes/java/util/concurrent/ExecutorCompletionService.java ! src/share/classes/java/util/concurrent/ExecutorService.java ! src/share/classes/java/util/concurrent/Executors.java ! src/share/classes/java/util/concurrent/ForkJoinPool.java ! src/share/classes/java/util/concurrent/ForkJoinTask.java ! src/share/classes/java/util/concurrent/ForkJoinWorkerThread.java ! src/share/classes/java/util/concurrent/Future.java ! src/share/classes/java/util/concurrent/FutureTask.java ! src/share/classes/java/util/concurrent/LinkedBlockingDeque.java ! src/share/classes/java/util/concurrent/LinkedBlockingQueue.java ! src/share/classes/java/util/concurrent/LinkedTransferQueue.java ! src/share/classes/java/util/concurrent/Phaser.java ! src/share/classes/java/util/concurrent/PriorityBlockingQueue.java ! src/share/classes/java/util/concurrent/RecursiveAction.java ! src/share/classes/java/util/concurrent/RecursiveTask.java ! src/share/classes/java/util/concurrent/RejectedExecutionException.java ! src/share/classes/java/util/concurrent/RejectedExecutionHandler.java ! src/share/classes/java/util/concurrent/RunnableFuture.java ! src/share/classes/java/util/concurrent/RunnableScheduledFuture.java ! src/share/classes/java/util/concurrent/ScheduledExecutorService.java ! src/share/classes/java/util/concurrent/ScheduledFuture.java ! src/share/classes/java/util/concurrent/ScheduledThreadPoolExecutor.java ! src/share/classes/java/util/concurrent/Semaphore.java ! src/share/classes/java/util/concurrent/SynchronousQueue.java ! src/share/classes/java/util/concurrent/ThreadFactory.java ! src/share/classes/java/util/concurrent/ThreadLocalRandom.java ! src/share/classes/java/util/concurrent/ThreadPoolExecutor.java ! src/share/classes/java/util/concurrent/TimeUnit.java ! src/share/classes/java/util/concurrent/TimeoutException.java ! src/share/classes/java/util/concurrent/TransferQueue.java ! src/share/classes/java/util/concurrent/atomic/AtomicBoolean.java ! src/share/classes/java/util/concurrent/atomic/AtomicInteger.java ! src/share/classes/java/util/concurrent/atomic/AtomicIntegerArray.java ! src/share/classes/java/util/concurrent/atomic/AtomicIntegerFieldUpdater.java ! src/share/classes/java/util/concurrent/atomic/AtomicLong.java ! src/share/classes/java/util/concurrent/atomic/AtomicLongArray.java ! src/share/classes/java/util/concurrent/atomic/AtomicLongFieldUpdater.java ! src/share/classes/java/util/concurrent/atomic/AtomicMarkableReference.java ! src/share/classes/java/util/concurrent/atomic/AtomicReference.java ! src/share/classes/java/util/concurrent/atomic/AtomicReferenceArray.java ! src/share/classes/java/util/concurrent/atomic/AtomicReferenceFieldUpdater.java ! src/share/classes/java/util/concurrent/atomic/AtomicStampedReference.java ! src/share/classes/java/util/concurrent/atomic/package-info.java ! src/share/classes/java/util/concurrent/locks/AbstractOwnableSynchronizer.java ! src/share/classes/java/util/concurrent/locks/AbstractQueuedLongSynchronizer.java !
Re: Review request for removal of lint warnings in DriverManager, CR 7034656
And the big man in the sky opens his book of good deeds, looks up Lance, and adds 4/7/2011 - Lance fixed warning errors. ;^) Thanks! -kto On Apr 7, 2011, at 6:48 AM, Lance Andersen - Oracle wrote: Hi all, This is a request for a review of the changes to remove the lint warnings for DriverManager. The diff is at http://cr.openjdk.java.net/~lancea/7034656/. Thank you. Regards, Lance Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
hg: jdk7/tl/jdk: 7034656: Address lint warnings for DriverManager
Changeset: 5137806a3e34 Author:lancea Date: 2011-04-07 11:25 -0400 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/5137806a3e34 7034656: Address lint warnings for DriverManager Reviewed-by: alanb, forax, ohair ! src/share/classes/java/sql/DriverManager.java
hg: jdk7/tl/jdk: 7029048: (launcher) fence the launcher against LD_LIBRARY_PATH
Changeset: d8dfd1a0bd8d Author:ksrini Date: 2011-04-07 12:06 -0700 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/d8dfd1a0bd8d 7029048: (launcher) fence the launcher against LD_LIBRARY_PATH Reviewed-by: mchung, ohair ! src/share/bin/jli_util.h ! src/solaris/bin/java_md.c ! test/tools/launcher/ExecutionEnvironment.java + test/tools/launcher/Test7029048.java ! test/tools/launcher/TestHelper.java
Re: Need reviewer: 7034700: simple fix
Looks fine to me. -kto On Apr 7, 2011, at 12:05 PM, Kumar Srinivasan wrote: Hi, This fixes a build issue in non-product builds, with the fix for 7033954. During the pack200 build the wrong mapfile is picked up due to the idiosyncrasy of the makefile. Thus the fix is simply and explicitly instructing Mapfile-vers.gmk not to use a mapfile for a non-product build. http://cr.openjdk.java.net/~ksrini/7034700/webrev.0/ Thanks Kumar PS: the previous change which activates mapfile checking, for reference : http://cr.openjdk.java.net/~ksrini/7033954/webrev.0/make/com/sun/java/pack/Makefile.udiff.html
Re: Suspected regression: fix for 6735255 causes delay in GC of ZipFile InputStreams, increase in heap demand
Hi Neil, Neil Richards said the following on 04/07/11 23:30: On Mon, 2011-04-04 at 09:04 +1000, David Holmes wrote: 1. If a call to close() occurs around the same time as finalization occurs then the finalizer thread will set inFinalizer to true, at which point the thread executing close() can see it is true and so may skip the releaseInflater(inf) call. 2. As isClosed is not volatile, and available() is not synchronized, a thread calling available() on a closed stream may not see that it has been closed and so will likely encounter an IOException rather than getting a zero return. Even if #1 is not a practical problem I'd be inclined to make the finalizer synchronized as well. By doing that you're effectively ensuring that premature finalization of the stream won't happen. I tend to agree, especially as it also makes the intention of the code clearer. For #2 it is a tricky call. If you don't actually expect the stream object to be used by multiple threads then using a synchronized block to read isClosed will be cheap in VMs that go to great effort to reduce the cost of unnecessary synchronization (eg biased-locking in hotspot). Otherwise making isClosed volatile is likely the better option. The check at the start of available() guards the logic beyond (which uses a value from the inflater object, which would not be valid if the stream has been closed()). Because of this, I think it would be clearer to synchronize the available() method too. As you say, it is extremely likely that, in practice, this synchronization will never be contended, and so won't impact performance significantly (in modern VMs). Please find below an updated changeset with these modifications, Thanks for your advice in this, No problem. From a synchronization perspective this all seems fine now. Cheers, David
advice review requested for 6896297, race condition in rmid causing JCK failure
Hi Core-Libs developers, I'd like to solicit some advice and discussion about this bug and a potential fix I'm cooking for it. Here is the bug report; it contains details about the problem and my analysis of it: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6896297 and here's a webrev of the fix I'm working on: http://cr.openjdk.java.net/~smarks/reviews/6896297/webrev.0/ Briefly, the problem is incorrect synchronization of groupTable, a HashMap field of an Activation object. The code mostly locks groupTable around any access to it. However, no such locking is done during serialization. If the groupTable is modified while it's being serialized, ConcurrentModificationException occurs. The obvious fix would be to use ConcurrentHashMap instead of Hashmap and to remove the external locking entirely. Unfortunately this will change the serialized representation of the Activation object, which I'm not sure is acceptable. Assuming that we can't change the serialized represenation, the alternative approach would be to make sure that locking is done properly during serialization. This is fairly easy to do by locking groupTable in a writeObject() method. Unfortunately, this introduces a deadlock. This deadlock occurs because, with this modification, there are now paths through the code that take locks in the opposite order. Specifically, the addLogRecord() method locks the log object and then (via serialization and the newly added writeObject() method) locks groupTable. However, the unregisterGroup() method locks groupTable and calls GroupEntry.unregisterGroup() which logs the event, which takes a lock on the log. After some analysis, I've determined that the call to GroupEntry.unregisterGroup() can be moved outside the synchronization of groupTable. This removes the ordering problem. With these fixes in place (the state of the webrev above) I can get several hundred successful test runs with neither ConcurrentModificationException nor any deadlocks. Of course, that doesn't mean the change is correct. :-) Questions: 1. Is there a requirement that the serialized form of Activation remain unchanged? If we can change it, we might as well just use ConcurrentHashMap instead of HashMap. 2. Is my lock ordering analysis correct? I've pored over the code, but not really being familiar with any RMI concepts, I'm not sure I have it right. I've written this analysis into a big comment I've added to the code. 3. There is also a potential concurrency issue with idTable, which is used similarly to groupTable. I haven't seen a test failure from it though. It seems sensible to add a lock for it in Activation.writeObject() though. I don't particularly like nesting the locks of idTable and groupTable, but locking them separately would require serializing the Activation object field by field instead of simply using defaultWriteObject(). Is this a reasonable approach? Thanks for any advice or comments. s'marks
Re: Suspected regression: fix for 6735255 causes delay in GC of ZipFile InputStreams, increase in heap demand
Neil, It appears it might not be necessary to do the finalize() in ZipFileInflaterInputStream. The ZipFileInflaterInputStream itself does not directly hold any native resource by itself that needs to be released at the end of its life circle, if not closed explicitly. The native resource/ memory that need to be taken care of are held by its fields inf and zfin, which should be finalized by the corresponding finalize() of their own classes (again, if not closed explicitly), when their outer ZFIIS object is unreachable. The Inflater class has its own finalize() implemented already to invoke its cleanup method end(), so the only thing need to be addressed is to add the finalize() into ZipFileInputStream class to call its close(), strictly speaking this issue is not the regression caused by #6735255, we have this leak before #6735255. Also, would you like to consider to use WeakHeapMapInputStream, Void instead of handling all the weak reference impl by yourself, the bonus would be that the stalled entries might be cleaned up more frequently. Sherman On 04/07/2011 06:30 AM, Neil Richards wrote: On Mon, 2011-04-04 at 09:04 +1000, David Holmes wrote: 1. If a call to close() occurs around the same time as finalization occurs then the finalizer thread will set inFinalizer to true, at which point the thread executing close() can see it is true and so may skip the releaseInflater(inf) call. 2. As isClosed is not volatile, and available() is not synchronized, a thread calling available() on a closed stream may not see that it has been closed and so will likely encounter an IOException rather than getting a zero return. Even if #1 is not a practical problem I'd be inclined to make the finalizer synchronized as well. By doing that you're effectively ensuring that premature finalization of the stream won't happen. I tend to agree, especially as it also makes the intention of the code clearer. For #2 it is a tricky call. If you don't actually expect the stream object to be used by multiple threads then using a synchronized block to read isClosed will be cheap in VMs that go to great effort to reduce the cost of unnecessary synchronization (eg biased-locking in hotspot). Otherwise making isClosed volatile is likely the better option. The check at the start of available() guards the logic beyond (which uses a value from the inflater object, which would not be valid if the stream has been closed()). Because of this, I think it would be clearer to synchronize the available() method too. As you say, it is extremely likely that, in practice, this synchronization will never be contended, and so won't impact performance significantly (in modern VMs). Please find below an updated changeset with these modifications, Thanks for your advice in this, Neil
Re: advice review requested for 6896297, race condition in rmid causing JCK failure
Hi Stuart, I can't answer your specific questions as I'm not familiar with the RMI infrastructure either. Taking the lock in writeObject and moving group.unregister out of the sync block to avoid deadlock seems reasonable. The main question to ask with such a move is whether the temporary inconsistency in state is something that can be observed and cause a problem. The lock ordering analysis seems reasonable. I do note that the startupLock protocol seems suspicious as there is a race with detection of the lock value. I assume init itself can never be invoked by more than one thread ;-) It may be that the object can only become accessible concurrently at some time during init (ie it gets published) in which case the protocol will work fine - but you really need a full understanding of the lifecycle of the objects to determine that. That aside, using ConcurrentHashMap would simplify the solution to the current problem. If you are concerned about the serialized form then you could define writeObject and readObject such that they convert between CHM and plain HM using the serialized-fields API. Cheers, David Stuart Marks said the following on 04/08/11 08:19: Hi Core-Libs developers, I'd like to solicit some advice and discussion about this bug and a potential fix I'm cooking for it. Here is the bug report; it contains details about the problem and my analysis of it: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6896297 and here's a webrev of the fix I'm working on: http://cr.openjdk.java.net/~smarks/reviews/6896297/webrev.0/ Briefly, the problem is incorrect synchronization of groupTable, a HashMap field of an Activation object. The code mostly locks groupTable around any access to it. However, no such locking is done during serialization. If the groupTable is modified while it's being serialized, ConcurrentModificationException occurs. The obvious fix would be to use ConcurrentHashMap instead of Hashmap and to remove the external locking entirely. Unfortunately this will change the serialized representation of the Activation object, which I'm not sure is acceptable. Assuming that we can't change the serialized represenation, the alternative approach would be to make sure that locking is done properly during serialization. This is fairly easy to do by locking groupTable in a writeObject() method. Unfortunately, this introduces a deadlock. This deadlock occurs because, with this modification, there are now paths through the code that take locks in the opposite order. Specifically, the addLogRecord() method locks the log object and then (via serialization and the newly added writeObject() method) locks groupTable. However, the unregisterGroup() method locks groupTable and calls GroupEntry.unregisterGroup() which logs the event, which takes a lock on the log. After some analysis, I've determined that the call to GroupEntry.unregisterGroup() can be moved outside the synchronization of groupTable. This removes the ordering problem. With these fixes in place (the state of the webrev above) I can get several hundred successful test runs with neither ConcurrentModificationException nor any deadlocks. Of course, that doesn't mean the change is correct. :-) Questions: 1. Is there a requirement that the serialized form of Activation remain unchanged? If we can change it, we might as well just use ConcurrentHashMap instead of HashMap. 2. Is my lock ordering analysis correct? I've pored over the code, but not really being familiar with any RMI concepts, I'm not sure I have it right. I've written this analysis into a big comment I've added to the code. 3. There is also a potential concurrency issue with idTable, which is used similarly to groupTable. I haven't seen a test failure from it though. It seems sensible to add a lock for it in Activation.writeObject() though. I don't particularly like nesting the locks of idTable and groupTable, but locking them separately would require serializing the Activation object field by field instead of simply using defaultWriteObject(). Is this a reasonable approach? Thanks for any advice or comments. s'marks
Re: Code review request for 6998871 Support making the Throwable.stackTrace field immutable
Hi Rémi. Rémi Forax wrote: On 04/07/2011 08:29 AM, Joe Darcy wrote: Hello. Returning to some earlier work, I've developed a proposed fix for 6998871 Support making the Throwable.stackTrace field immutable http://cr.openjdk.java.net/~darcy/6998871.2/ One constructor of Throwable now takes an additional boolean argument to make the stack trace information immutable. Analogous constructors are added to Exception, RuntimeException, and Error. Mandy and David have already reviewed the change; I'm interested in getting additional feedback on the design of the API. Cheers, -Joe Joe, I don't think you need the sentinel in the serialized form, you have only two states: an immutable stacktrace (stacktrace == null) or a stacktrace. I think it's better to don't serialize the field stacktrace if the stacktrace is immutable. Also, FILLED_IN_STACK is not necessary, you can use EMPTY_STACKinstead, or if you find it hard to understand, at least FILLED_IN_STACK should be an empty array. Rémi Here is an explanation and rationale of the protocol in more detail. As far as serialization goes, there are three cases of interest where new means JDK 7 and old means any release prior to JDK 7 1) Object serialized on *new* platform has same semantics after being deserialized on *new* platform. 2) Object serialized on *old* platform has same semantics after being deserialized on *new* platform. 3) Object serialized on *new* platform has reasonable semantics after being deserialized on *old* platform. (And of course when both source and destination platforms are old, JDK 7 isn't involved.) For point 1), a logically equivalent object should result, which in this case means the cause, stack trace, stack trace, message, etc. should be the same on the old and new objects. For 2), if an exception object serialized prior to the stack trace facility being added was deserialized in JDK 7, that is logically equivalent to there being not stack trace information, so an empty stack is a better result than an immutable stack. I've updated the readObject method accordingly. For 3), since null was not a valid value for stackTrace in the past, some other sentinel object should be written in out in the serial form to denote such a value, which is the role played by STACK_TRACE_SENTINEL as used in the writeObject method. A marker value other than EMPTY_STACK is needed for the following reason since the stack trace information is spread amongst two fields, backtrace and stackTrace. The transient backtrace field pre-dates the programmatic stack facility being added; that facility uses the stackTrace field. If fillInStackTrace is called *after* a call to setStackTrace, the real stack information is held in the backtrace field as written by fillInStackTrace. The FILLED_IN_STACK value alerts the getourstacktrace method to this situation. Revised webrev: http://cr.openjdk.java.net/~darcy/6998871.3/ Thanks, -Joe
Re: Code review request for 6998871 Support making the Throwable.stackTrace field immutable
On 04/07/11 16:26, Joe Darcy wrote: Revised webrev: http://cr.openjdk.java.net/~darcy/6998871.3/ Looks good. Mandy
Re: Codereview request for 7033561: Missing Unicode Script aliases
Hi Sherman, The fix looks good to me. Thanks, -- Yuka (11/04/07 5:16), Xueming Shen wrote: It appears the aliases mapping for Character.UnicodeScript is not updated accordingly when we upgraded the Unicode support to 6.0 for JDK7. The difference between the previous version (5.2) and 6.0 of the aliases are these 3 missing names reported in #7033561. The webrev with the change is at http://cr.openjdk.java.net/~sherman/7033561/webrev Thanks, Sherman
hg: jdk7/tl/jdk: 7034700: (unpack200) build fails with fastdebug builds
Changeset: 587e968b03ee Author:ksrini Date: 2011-04-07 17:08 -0700 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/587e968b03ee 7034700: (unpack200) build fails with fastdebug builds Reviewed-by: ohair ! make/com/sun/java/pack/Makefile
Re: Code review request for 6998871 Support making the Throwable.stackTrace field immutable
Joe Darcy wrote: Hi Rémi. Rémi Forax wrote: On 04/07/2011 08:29 AM, Joe Darcy wrote: Hello. Returning to some earlier work, I've developed a proposed fix for 6998871 Support making the Throwable.stackTrace field immutable http://cr.openjdk.java.net/~darcy/6998871.2/ One constructor of Throwable now takes an additional boolean argument to make the stack trace information immutable. Analogous constructors are added to Exception, RuntimeException, and Error. Mandy and David have already reviewed the change; I'm interested in getting additional feedback on the design of the API. Cheers, -Joe Joe, I don't think you need the sentinel in the serialized form, you have only two states: an immutable stacktrace (stacktrace == null) or a stacktrace. I think it's better to don't serialize the field stacktrace if the stacktrace is immutable. Also, FILLED_IN_STACK is not necessary, you can use EMPTY_STACKinstead, or if you find it hard to understand, at least FILLED_IN_STACK should be an empty array. Rémi Here is an explanation and rationale of the protocol in more detail. As far as serialization goes, there are three cases of interest where new means JDK 7 and old means any release prior to JDK 7 1) Object serialized on *new* platform has same semantics after being deserialized on *new* platform. 2) Object serialized on *old* platform has same semantics after being deserialized on *new* platform. 3) Object serialized on *new* platform has reasonable semantics after being deserialized on *old* platform. (And of course when both source and destination platforms are old, JDK 7 isn't involved.) For point 1), a logically equivalent object should result, which in this case means the cause, stack trace, stack trace, message, etc. should be the same on the old and new objects. For 2), if an exception object serialized prior to the stack trace facility being added was deserialized in JDK 7, that is logically equivalent to there being not stack trace information, so an empty stack is a better result than an immutable stack. I've updated the readObject method accordingly. For 3), since null was not a valid value for stackTrace in the past, some other sentinel object should be written in out in the serial form to denote such a value, which is the role played by STACK_TRACE_SENTINEL as used in the writeObject method. A marker value other than EMPTY_STACK is needed for the following reason since the stack trace information is spread amongst two fields, backtrace and stackTrace. The transient backtrace field pre-dates the programmatic stack facility being added; that facility uses the stackTrace field. If fillInStackTrace is called *after* a call to setStackTrace, the real stack information is held in the backtrace field as written by fillInStackTrace. The FILLED_IN_STACK value alerts the getourstacktrace method to this situation. Revised webrev: http://cr.openjdk.java.net/~darcy/6998871.3/ Thanks, -Joe PS A little more detail here, exception objects can be created via deserialization as well as by a constructor. As currently written, the writeObject method uses EMPTY_STACK for both a zero-length incoming stack and a null stackTrace pointer (which can result from an throwable object serialized from a JDK release before the stack trace facility was added). The native fillInStackTrace method writes a null into the stackTrace field to indicate it is no longer valid; this goes against the new protocol and the Java-level of fillInStackTraces needs to write a non-null dirty value into the field to signal to getOurStackTrace that the stack trace array needs to be recomputed if requested. Therefore, if fillInStackTrace were called on one of these deserialized objects to logically replace its stack trace, using EMPTY_STACK would not allow the getOutStackTrace code to know that the stack trace had logically been replaced by a new value in backtrace since EMPY_STACK could result from serialization. The serialization code could use EMPTY_STACK.clone() instead of EMPTY_STACK, which would break the object equality being tested in getOurStackTrace. I'll consider making this change in the Throwable.readObject method. Cheers, -Joe
