hg: jdk8/tl/jdk: 7105952: Improve finalisation for FileInputStream/FileOutputStream/RandomAccessFile
Changeset: 9dd994f319ee Author:coffeys Date: 2011-11-11 10:08 + URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/9dd994f319ee 7105952: Improve finalisation for FileInputStream/FileOutputStream/RandomAccessFile Reviewed-by: alanb ! src/share/classes/java/io/FileInputStream.java ! src/share/classes/java/io/FileOutputStream.java ! src/share/classes/java/io/RandomAccessFile.java ! src/solaris/classes/java/io/FileDescriptor.java ! src/windows/classes/java/io/FileDescriptor.java - test/java/io/FileDescriptor/FileChannelFDTest.java + test/java/io/FileDescriptor/Sharing.java - test/java/io/etc/FileDescriptorSharing.java
Fwd: RE: Code Review Request for 4533691 (add Collections.EMPTY_SORTED_SET)
Hi Jason. I'm the engineer implementing this RFE. Mike is committing the fix for me as I don't yet have commit rights to the repository. Please see my comments inline. Darryl Original Message Subject: RE: Code Review Request for 4533691 (add Collections.EMPTY_SORTED_SET) Date: Fri, 4 Nov 2011 12:52:54 -0500 From: Jason Mehrens jason_mehr...@hotmail.com To: mike.dui...@oracle.com, david.hol...@oracle.com CC: core-libs-dev@openjdk.java.net Mike, These 'simple' classes are really hard to get right. Here is my review of the change: 1. Why not extend EmptySet? You wouldn't have to implement so many methods. Good point. 2. The comparator method is using raw types. The SortedSet.comparator() method spec allows returning of null. 3. The readResolve method comment is just wrong. Create a default access static final reference named EMPTY_SORTED_SET inside of the EmptySortedSet and use that in readResolve and in Collections.emptySortedSet. Yes. This comment was left over from when the EMPTY_SORTED_SET field was there. I missed this, thanks. 4. Only the IAE if statement is need for your range checks. NPE and CCE will occur in that if statement by default. CCE lacks a descriptive message the you get if you used Class.cast or just an implicit cast. True. I'm trying to be clear in the code that the parameters are checked for these things. Is this an optimization/style issue? What is the preference here? 5. What if I want to create an empty set sorted set with supplied comparator? Extend EmptySortedSet and override the comparator method. I believe most uses of EmptySortedSet will not want to supply their own Comparator. I can support a suppliable Comparator if there's enough interest. 6. Why not implement an EmptyNavigableSet instead since the bug was entered before 1.6? Is NavigableSet preferrable to SortedSet? There is currently no request for EmptyNavigableSet, just EmptySortedSet. Did I make the case for a follow up review? :) Regards, Jason Subject: Re: Code Review Request for 4533691 (add Collections.EMPTY_SORTED_SET) From:mike.dui...@oracle.com Date: Fri, 4 Nov 2011 09:29:09 -0700 To:david.hol...@oracle.com CC:core-libs-dev@openjdk.java.net On Nov 3 2011, at 17:40 , David Holmes wrote: Mike, I see that you have pushed a version of this change and that I was listed as a reviewer. However I never saw an updated webrev and there was no response to my query below. In fact I never saw any response to any of the reviewers comments on this. I missed your question about the range on an empty set. My comments below. Removing EMPTY_SORTED_SET was the only other comment to my knowledge. Darryl adapted the patch and EMPTY_SORTED_SET was not part of the commit. Since the change was removal of a newly proposed field an additional review cycle wasn't thought to be necessary. Perhaps incorrectly?
Re: Code Review Request for 6578042
The reason I used the Object's toString method is that there may indeed be an object which is not a String which is stored as a value (this was the reason for the bug report). In this case, the remove method will in fact remove the property. Returning null if the value is not a String gives the impression that there was no property with that key when the property may have been there and may in fact have been removed. Using toString gives a best effort to notify the caller that the property was removed, even if the value is not a String. This to me is a bit different from getProperty. Returning null from getProperty gives the impression there is no property with the specified key, which may be false if the value is not a String, but you could argue that a non-String value is not a valid property anyway and should not be returned from getProperty. However, clearProperty will actually remove an invalid property. I will add to the Javadoc comment for the getProperty(String key) method that null may be returned if the property value is not a String. I would prefer it if Properties didn't allow non-String keys and values at all since they're supposed to be Strings (by API implication) and that Properties not extend Hashtable or extends HashtableString, String, but I digress. Darryl On Sun 06 Nov 2011 02:45:14 AM PST, Alan Bateman wrote: On 02/11/2011 19:59, David Holmes wrote: I'm not seeing a distinction in those two statements. Both expect to return the property value for a given key; both assume a valid value is a String. clearProperty throws ClassCastException if the assumption doesn't hold; getProperty instead returns null. The distinction is value vs. string value, where the latter is interpreted by Darryl's patch to be the String representation. While that avoids the CCE, it is clearly inconsistent with System.getProperty that I see is also specified to return the string value. So I think we have to dismiss this approach. I think this leaves two options: 1. Document existing behavior, meaning @throws CCE (several of Properties methods already have this). 2. Change the method so that it is consistent with getProperty and so returns null if the property value is not a String. While option 2 is clearly a change then I think it's worth exploring to understand the impact. Properties has always had a note in its javadoc to warn users that it inherits from Hashtable and it has a note to strongly discourage inserting entries that have a key or value that is not a String. My guess (and this is purely a guess) is that System.clearProperty is not used very often and so the likelihood of someone abusing the system properties and using clearProperty is probably low. I see the bug on bugs.sun.com doesn't have any votes or comments which suggests that this one doesn't have an angry mob either. Darryl - since you have decided to tackle this one then I would suggest looking into this further and coming back with a recommendation. I would also suggest that as part of the patch that the javadoc for System.getProperty and Properties.getProperty be clarified so that it's clear that they return null when the properties have been compromised. -Alan.
Re: CR 6860309 - solaris timing issue on thread startup
Hi Gary, On 12/11/2011 2:56 AM, Gary Adams wrote: CR 6860309 - TEST_BUG: Insufficient sleep time in java/lang/Runtime/exec/StreamsSurviveDestroy.java A timing problem is reported for slow solaris systems for this test to start up a process and systematically torture the underlying threads processing data from the running process. On my fast solaris machine I can not reproduce the error, but it is reasonable to assume that on a slower machine there could be scheduling issues that could delay the thread startup past the designated 100 millisecond delay in the main thread. This webrev suggests gating the process destruction until both worker threads are alive. http://cr.openjdk.java.net/~gadams/6860309/ This doesn't achieve anything I'm afraid. isAlive() will return true once start() has been invoked on the thread (it doesn't depend on the new thread getting scheduled to run), hence there will only be a single call to sleep - no different from today. If the issue is that c1 and c2 have not performed some action necessary before the process interaction then that action needs to be synchronized with explicitly. David -
