Re: Race condition in TimerTask KillThread test
On 04/11/2011 15:52, Gary Adams wrote:
:
I'll attempt a simple fix for the KillThread case by replacing :
Thread.sleep(100);
with a simple loop
do {
Thread.sleep(100);
} while (waiting);
where the TimerTask runnable will clear the flag
with "waiting = false " once it has been launched.
I don't think this will guarantee that the timer thread will have
terminated before the main thread schedules the second event. I think
this test would be robust if you capture a reference to the timer thread
in the first task and have the main thread wait or poll until it has a
reference to the thread. Once it has a reference to the thread then it
can wait for it to terminate before attempting to schedule the second task.
-Alan.
core-libs-dev@openjdk.java.net
Yep. Please file a bug. Naoto On 11/4/11 12:39 PM, Alan Bateman wrote: As to the ArrayIndexOutOfBoundsException, that does seem to be a bug as I assume that getClassContext is returning an array of one element corresponding to the caller frame (getBundle in this case).
core-libs-dev@openjdk.java.net
On 04/11/2011 12:03, Heiko Wagner wrote: Hi Tom, thanks for your reply. I am using JNI in a different,propably never designed to be used that way, kind of scenario. I use JNI to bring Java to a legacy Smalltalk based product[1]. The Smalltalk code does directly invoke the JNI calls from its own Smalltalk VM main thread. So the Java VM is not aware of the calling thread and has no call stack information. Currently, I am using ClassLoader.getSystemClassLoader(), which perfectly works for me. I have no idea if this makes sense in gerneal, but for me a MissingResourceBundleException in my use case, would be what I propably would have expected, instead of a ArrayIndexOutOfBoundsException, so maybe a additional size check on the call stack array size would make sense. The thread must be known to the VM as otherwise you wouldn't have a JNIEnv and wouldn't be able to call the method (so I'll bet the thread is attaching via AttachCurrentThread somewhere). As to the ArrayIndexOutOfBoundsException, that does seem to be a bug as I assume that getClassContext is returning an array of one element corresponding to the caller frame (getBundle in this case). -Alan.
RE: Code Review Request for 4533691 (add Collections.EMPTY_SORTED_SET)
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. 2. The comparator method is using raw types. 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. 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. 5. What if I want to create an empty set sorted set with supplied comparator? 6. Why not implement an EmptyNavigableSet instead since the bug was entered before 1.6? 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 4533691 (add Collections.EMPTY_SORTED_SET)
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?
Mike
> David
>
> On 31/10/2011 10:52 AM, David Holmes wrote:
>> Darryl,
>>
>> On 29/10/2011 8:44 AM, Darryl Mocek wrote:
>>> Hello. Please review this patch to add empty sorted set to the
>>> Collections class. Test case provided.
>>>
>>> Webrev: http://cr.openjdk.java.net/~mduigou/4533691/1/webrev/
>>
>> This was an RFE from 2001 - pre-generics. I don't think it makes sense
>> to add to the pre-generics pollution by defining EMPTY_SORTED_SET.
>>
>>> Additional Notes to Reviewers:
>>> The sets resulting from tailSet() headSet() and subSet() normally
>>> include the range which was used to create them. Using these methods
>>> with emptySortedSet() does not currently set a range on the resulting
>>> sets.
>>
>> What is the range on an empty set?
The results of Collections.emptySortedSet() will behave differently than
Collections.unmodifiableSortedSet(new TreeSet()) for cases involving headSet,
tailSet and subSet. These three operations remember the range used to create
the sub set in addition to providing the elements within that range. The range
is considered in a few cases such add item addition (added items must be within
the range of the subset) and any subset created from a range sub set must lie
within the range. Relevant to emptySortedSet is the creation of sub sets off of
a ranged sub set.
public static void main(String[] args) {
SortedSet foo = Collections.unmodifiableSortedSet(new
TreeSet());
SortedSet two = foo.headSet(Double.valueOf(2.0)); // range < 2.0
SortedSet one = two.headSet(Double.valueOf(1.0));
try {
SortedSet three = two.headSet(Double.valueOf(3.0)); // IAE
"toKey out of range"
} catch (IllegalArgumentException expected) {
expected.printStackTrace(System.err);
}
two = foo.tailSet(Double.valueOf(2.0)); // range >= 2.0
try {
one = two.tailSet(Double.valueOf(1.0)); // IAE "fromKey out of range"
} catch (IllegalArgumentException expected) {
expected.printStackTrace(System.err);
}
SortedSet three = two.tailSet(Double.valueOf(3.0));
two = foo.subSet(Double.valueOf(2.0),Double.valueOf(3.0)); // 2.0 <= range
< 3.0
try {
one = two.subSet(Double.valueOf(1.0),Double.valueOf(2.0)); // IAE
"fromKey out of range"
} catch (IllegalArgumentException expected) {
expected.printStackTrace(System.err);
}
three = two.subSet(Double.valueOf(2.0),Double.valueOf(3.0));
}
This program throws three IAE exceptions. Replacing the first statement with:
SortedSet foo = Collections.emptySortedSet();
would result in no IAE exceptions because they emptySortedSet() does not track
ranges for sub sets derived with headSet(), tailSet() and subSet(). The range
on emptySortedSet and derived sets is always fully open.
Mike
Race condition in TimerTask KillThread test
I'm taking a look at some older timing based bugs that may cause problems
on slower machines
6818464: TEST_BUG: Timeout values in several java/util tests are insufficient
I'd like to split this bug into two, based on the example problems that are
mentioned in the bug report.
The first example in
java/util/Timer/KillThread.java
is a legitimate race condition. The code only will work
correctly if the processing for the TimerTask schedules
and fires the runnable within the hard coded
100 milliseconds. This can be fixed with a local variable
to synchronize when the the second operation can be
attempted. (Hard coded sleep times are rarely correct
when dealing with slower devices.)
In the second example the test instructions present a timeout
to be enforced by the outer test harness. e.g.
@run main/timeout=20 StoreDeadLock
This type of test limit is easily addressed on slower machines
using the test harness timefactor to scale the acceptable
test run time.
I'll attempt a simple fix for the KillThread case by replacing :
Thread.sleep(100);
with a simple loop
do {
Thread.sleep(100);
} while (waiting);
where the TimerTask runnable will clear the flag
with "waiting = false " once it has been launched.
Timing bugs
I've started to look at timing related bugs that have been open
for a while, but have not had sufficient priority to make it to the
top of the list of bugs to be fixed. Thought I'd start with some
low hanging fruit with simple bug fixes.
6731620: TEST_BUG: java/util/Timer/Args.java is too optimistic about the
execution time of System.out.printf
This seems like a simply problem to avoid two calls to get the current time
and to eliminated the time to process the print statement
from the evaluation of the test elapsed time.
Replacing this sequence ;
System.out.printf("elapsed=%d%n", System.currentTimeMillis() - start);
check(System.currentTimeMillis() - start < 500);
with
elapsed = System.currentTimeMillis() - start;
System.out.printf("elapsed=%d%n", elapsed);
check(elapsed < 500);
I plan to test the fix on a 300MHz linux/arm device.
I'll provide a proper webrev as soon as I have author rights
confirmed. I'm looking for reviewer and a committer,
once I get the fix tested locally.
Thanks
Gary Adams
Re: code review request : 7105952: Improve finalisation for FileInputStream/FileOutputStream/RandomAccessFile
ok, so updated webrev at : http://cr.openjdk.java.net/~coffeys/webrev.7105952.2/ some minor modifications : * "closed" variable made private * hg mv instead of rm/add for testcase * testcase padded up some more. * comments changed to block style format in FileDescriptor. * some extra comments added to FileDescriptor to help read code I'll leave the javadoc changes for another bugID/CCC request. regards, Sean. On 02/11/11 06:35, Alan Bateman wrote: On 01/11/2011 09:47, Seán Coffey wrote: : Are you referring to the parent.close() call ? If otherParents is null, then we only have one reference to this FileDesriptor - i.e the Stream that has called close(). It's parent.close() that I'm wondering about. Suppose p1 is the parent, p2 is in otherParents. If p2.close is invoked then it looks like p1's close method will not be invoked, leaving p1 open and the underlying file descriptor closed. Any exception from releaser.close() becomes the main exception if one is seen there. Any exceptions from the earlier closes are then added as suppressed. I've run some tests on this and it looked to work as expected. (stack trace below) In this example then the IOException appears to be thrown suppressing one exception. That suppression exception in turn suppresses two others. I had expected that the primary IOException would have suppressed three exceptions. As I said, in practical terms this is much of a concern, just pointing out that it's not exactly as expected. I used hg rm/add. I guess hg mv would have worked too. You need to use hg mv so that the history can be followed. Also webrev handles it, at least before you create the changeset, so that you can easily see the changes. -Alan.
hg: jdk8/tl/langtools: 7104201: Refactor DocCommentScanner
Changeset: 56830d5cb5bb Author:mcimadamore Date: 2011-11-04 12:36 + URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/56830d5cb5bb 7104201: Refactor DocCommentScanner Summary: Add new Comment helper class to parse contents of comments in source code Reviewed-by: jjg ! src/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java ! src/share/classes/com/sun/tools/javac/parser/JavacParser.java ! src/share/classes/com/sun/tools/javac/parser/JavadocTokenizer.java ! src/share/classes/com/sun/tools/javac/parser/Tokens.java ! src/share/classes/com/sun/tools/javac/parser/UnicodeReader.java + test/tools/javac/depDocComment/DeprecatedDocComment4.java + test/tools/javac/depDocComment/DeprecatedDocComment4.out
core-libs-dev@openjdk.java.net
Hi Tom, thanks for your reply. I am using JNI in a different,propably never designed to be used that way, kind of scenario. I use JNI to bring Java to a legacy Smalltalk based product[1]. The Smalltalk code does directly invoke the JNI calls from its own Smalltalk VM main thread. So the Java VM is not aware of the calling thread and has no call stack information. Currently, I am using ClassLoader.getSystemClassLoader(), which perfectly works for me. I have no idea if this makes sense in gerneal, but for me a MissingResourceBundleException in my use case, would be what I propably would have expected, instead of a ArrayIndexOutOfBoundsException, so maybe a additional size check on the call stack array size would make sense. P.S. Despite my hefty "abuse" of JNI, it overall still works very well. Regards, Heiko [1] http://vst.ensm-douai.fr/Esug2008Media/uploads/1/APIS_ESUG.pdf (Slide 12-19) >I'm not a JNI expert (you are calling from a thread known to the JVM, >right?). However, getBundle is one of the magic methods listed in >section 6 (6-4) of the Java Secure Coding Guidelines[1] that depend upon >the immediate caller. If there isn't an immediate [Java] caller, that >isn't going to work. Which ClassLoader would you want getBundle to use? > >Tom
core-libs-dev@openjdk.java.net
On 04/11/11 09:58, Heiko Wagner wrote: This is caused by the fact that the ResourceBundle class tries to determine the actual ClassLoader using the getLoader() method, via accessing the hardcoded stack offset 2, which works when the getBundle() method is callen from Java, but in my case I invoke this method using JNI and the stack layout is different, so causing the exception. I know this code has been this way a for a long time, but I still get a beed feeling when the code relies on some hard code offsets in the call stack and I see no reason why invoking the ResourceBundle methods via JNI should be considered "illegal". I'm not a JNI expert (you are calling from a thread known to the JVM, right?). However, getBundle is one of the magic methods listed in section 6 (6-4) of the Java Secure Coding Guidelines[1] that depend upon the immediate caller. If there isn't an immediate [Java] caller, that isn't going to work. Which ClassLoader would you want getBundle to use? Tom [1]http://www.oracle.com/technetwork/java/seccodeguide-139067.html#6-0
core-libs-dev@openjdk.java.net
I hope this is the right list to post this. I encounter some behaviour, that
I am not sure whether this is correct. When invoking
ResourceBundle.getBundle("com.some.Bundle") I get a
ArrayIndexOutOfBoundsException.
This is caused by the fact that the ResourceBundle class tries to determine
the actual ClassLoader using the getLoader() method, via accessing the
hardcoded stack offset 2, which works when the getBundle() method is callen
from Java, but in my case I invoke this method using JNI and the stack
layout is different, so causing the exception.
I know this code has been this way a for a long time, but I still get a beed
feeling when the code relies on some hard code offsets in the call stack and
I see no reason why invoking the ResourceBundle methods via JNI should be
considered "illegal".
Despite the trivial workaround for this problem. i.e. explicitly specify the
ClassLoader with getBundle(String, Locale, ClassLoader), I would like to
hear your opinion on this.
Regards,
Heiko
