Re: Patch to expand tz checking scope in TimeZone_md.c
On 11/04/2011 01:26 PM, David Holmes wrote:
On 4/11/2011 2:53 PM, David Holmes wrote:
On 4/11/2011 2:13 PM, Masayoshi Okutsu wrote:
Probably the difference isn't documented. I tried Solaris 10 and Ubuntu
10.03. The difference still exists.
Solaris 10:
$ unset TZ
$ date
Fri Nov 4 13:04:45 JST 2011
$ TZ= date
Fri Nov 4 13:04:53 JST 2011
Ubuntu 10.04:
$ unset TZ
$ date
Fri Nov 4 13:05:50 JST 2011
$ TZ= date
Fri Nov 4 04:05:55 UTC 2011
When the TZ value is an empty string, Ubuntu uses UTC while Solaris
still looks up the system default.
I have to take back my comment regarding this not seeming to be platform
specific code - it is highly platform specific! It seems that on Linux
we are happy to report a TZ of but not so on Solaris. I presume this
is an attempt to keep Java's use of TZ consistent with how other apps
handle it on that platform. (environ(5) gives a little insight on
Solaris as to how TZ is used.)
So the key thing here is to not disturb the existing behaviour on either
linux or Solaris - which suggests the original patch. That said I'm not
convinced - given this is so platform specific - that simply treating
non-linux the same as Solaris is a reasonable thing to do. I think it
would be useful to see what the BSD/OSX port(s) had to do with this code
- if anything.
To answer my own queries BSD/OSX does
511 #if defined(__linux__) || defined(_ALLBSD_SOURCE)
512 if (tz == NULL) {
513 #else
514 #ifdef __solaris__
515 if (tz == NULL || *tz == '\0') {
516 #endif
517 #endif
so the suggested patch would at least not interfere.
Anyway this needs input from other core-libs folk. I didn't intend to
get quite so heavily involved. ;-)
David
-
David
-
Thanks,
Masayoshi
On 11/3/2011 4:16 PM, Jonathan Lu wrote:
Hi Masayoshi,
I did find some references about date-time related functions / TZ
variables on Linux but got only a few about Solaris, so could not see
any differences between those two platforms about the changes
described in my patch. Have you got any links or references about
these differences? I'm interested in it and may update the patch again
after reading them.
Thanks a lot!
- Jonathan
On 11/02/2011 10:27 PM, Masayoshi Okutsu wrote:
Hi Jonathan,
IIRC, the difference came from some behavioral difference between the
Linux and Solaris libc date-time functions and/or the date command,
and TimeZone_md.c tries to follow the difference. But the code was
written long ago. The difference may no longer exist.
Thanks,
Masayoshi
On 11/2/2011 8:39 PM, Jonathan Lu wrote:
On 11/02/2011 07:00 PM, David Holmes wrote:
On 2/11/2011 7:01 PM, Jonathan Lu wrote:
On 11/02/2011 04:56 PM, Jonathan Lu wrote:
Hi core-libs-dev,
In jdk/src/solaris/native/java/util/TimeZone_md.c, starting from
line
626, I found that the scope of #ifdef __solaris__ might be too
narrow, since it also works for some kind of OS which I'm
currently
working on, such as AIX.
So I suggest to just remove the '#ifdef __solaris__' and leave
the
#else to accommodate more conditions, see attachment
'patch.diff'. I
think this may enhance the cross-platform ability, any ideas
about
this modification?
Regards
- Jonathan Lu
I'm not sure why the attachment got filtered, here paste it to the
mail
content directly.
diff -r 4788745572ef src/solaris/native/java/util/TimeZone_md.c
--- a/src/solaris/native/java/util/TimeZone_md.c Mon Oct 17
19:06:53
2011 -0700
+++ b/src/solaris/native/java/util/TimeZone_md.c Thu Oct 20
13:43:47
2011 +0800
@@ -626,10 +626,8 @@
#ifdef __linux__
if (tz == NULL) {
#else
-#ifdef __solaris__
if (tz == NULL || *tz == '\0') {
#endif
-#endif
tz = getPlatformTimeZoneID();
freetz = tz;
}
I'm unclear why any of that code needs to be platform specific - is
an empty TZ string somehow valid on linux? I would have thought the
following would be platform neutral:
if (tz == NULL || *tz == '\0') {
tz = getPlatformTimeZoneID();
freetz = tz;
}
Hi David,
getenv(TZ) returns NULL when TZ environment variable is not set at
all and returns '\0' when TZ was exported as empty string. After
more checking for both cases, I agree with you, nothing useful can
be retrieved from that environment variable.
So I changed the patch to this,
diff -r 7ab0d613cd1a src/solaris/native/java/util/TimeZone_md.c
--- a/src/solaris/native/java/util/TimeZone_md.c Thu Oct 20 10:32:47
2011 -0700
+++ b/src/solaris/native/java/util/TimeZone_md.c Wed Nov 02 19:34:51
2011 +0800
@@ -623,13 +623,7 @@
tz = getenv(TZ);
-#ifdef __linux__
- if (tz == NULL) {
-#else
-#ifdef __solaris__
if (tz == NULL || *tz == '\0') {
-#endif
-#endif
tz = getPlatformTimeZoneID();
freetz = tz;
}
David
-
Regards
- Jonathan Lu
Regards
- Jonathan
Copy to bsd-port-dev and macosx-port-dev lists to see if anybody here
has some ideas about this issue.
-Jonathan
Re: Garbage collection race condition before final checks
On 08/11/2011 15:35, Gary Adams wrote: Here's another intermittent bug that is attributed to the garbage collection of the loggers before the final static check can be applied in the test. CR#7067691 : java/lang/management/PlatformLoggingMXBean/LoggingMXBeanTest.java failing intermittently I agree with Mandy that it would be great to do a quick audit of the other logging (and PlatformLoggingMXBean) tests to see if we have similar issues. I should explain that one reason why these test failures may not have been observed in the past is because most people ran jtreg in its default mode (called othervm mode, where each tests runs in its own VM). Nowadays we run the tests via the make file or use jtreg -agentvm so that tests are running sequentially in an agent VM. Not all areas can run in agent VM mode yet but for the areas that do then we get a good speed up as the startup cost is eliminated and also it's possible to have a pool of agent VMs to make use of all cores when doing test runs (-agentvm -concurrency:auto for example). However agent VM makes things less predictable and for these tests it means that the GC will be unpredictable which is why this test was failing only very intermittently. Anyway, the changes look fine to me. I agree with Mandy that many logger1 and logger2 could be instance variables. I would suggest proxy or something more readable rather than testp for the LoggingMXBean proxy. I also agree with Mandy's comment about adding the @bug. -Alan.
Re: A method with return type size_t returns negative value
Hello Alan, (Sorry that haven't check this for months) I check with the bug 7030624 but see no progress now. What's the current status? Shall we go on with a simple patch to fix the problem? On 2011/7/21 21:32, Alan Bateman wrote: Jing LV wrote: Ping, anyone notice this? :) My plan is that we'd search all source to find all kinds of such issues, and then refine them in a uniform solution. If jint is not good enough, how about ssize_t? ACK, saw your mails and will get back to you soon on this. -Alan
Re: Garbage collection race condition before final checks
On 11/8/11 11:13 PM, Mandy Chung wrote:
Gary,
Thanks for picking up this bug and fixing this intermittent issue.
PlatformLoggingMXBeanTest.java in the same directory has the same
issue. It'd be good to fix that with the same CR. These tests were
copied from test/java/util/logging/LoggingMXBeanTest.java. I haven't
looked in details but I wonder why the test/java/util/logging tests
don't have this intermittent issue and I suspect it holds a strong
reference.
I'll take a look at the other tests to see if they suffer from the
same issue. I was able to force the intermittent bugs to reproduce quickly
with a strategic GC in the worst possible place.
A couple comment to the fix:
On 11/8/2011 7:35 AM, Gary Adams wrote:
diff --git
a/test/java/lang/management/PlatformLoggingMXBean/LoggingMXBeanTest.java
b/test/java/lang/management/PlatformLoggingMXBean/LoggingMXBeanTest.java
---
a/test/java/lang/management/PlatformLoggingMXBean/LoggingMXBeanTest.java
+++
b/test/java/lang/management/PlatformLoggingMXBean/LoggingMXBeanTest.java
Please add the CR number to @bug tag.
Will do.
@@ -42,6 +42,9 @@
static String LOGGER_NAME_1 = com.sun.management.Logger;
static String LOGGER_NAME_2 = com.sun.management.Logger.Logger2;
static String UNKNOWN_LOGGER_NAME = com.sun.management.Unknown;
+static Logger logger1;
+static Logger logger2;
It'd be good to add a comment why you want to keep a strong reference
to the logger. Minor nit: could make them as instance variables as
they are part of the LoggingMXBeanTest instance.
I'll give it a try.
+static LoggingMXBeanTest testp;
public static void main(String[] argv) throws Exception {
MBeanServer mbs = ManagementFactory.getPlatformMBeanServer();
@@ -51,7 +54,7 @@
LoggingMXBean.class);
// test LoggingMXBean proxy
-LoggingMXBeanTest p = new LoggingMXBeanTest(proxy);
+testp = new LoggingMXBeanTest(proxy);
Could we make checkAttributes as an instance method to
LoggingMXBeanTest object so that you don't need the static variable?
My preference is to leave a test as intact as possible
from how the original test was written. e.g. do the minimum
required to make the test reliable. These regression tests
are meant to confirm that a particular issue does not slip
back into the code base. Changing checkAttributes to be
an instance method is not essential to getting the test to
work properly. Better to get 10 intermittent tests to work reliably
rather than get 1 test polished to perfection. $.02
// check if the attributes implemented by PlatformLoggingMXBean
// and LoggingMXBean return the same value
@@ -59,14 +62,18 @@
ManagementFactory.getPlatformMXBean(mbs,
PlatformLoggingMXBean.class);
checkAttributes(proxy, mxbean);
+
+logger1 = null;
+logger2 = null;
+testp = null;
With the above suggested change, I think these 3 lines are not needed.
OK
}
// same verification as in java/util/logging/LoggingMXBeanTest2
public LoggingMXBeanTest(LoggingMXBean mbean) throws Exception {
-Logger logger1 = Logger.getLogger( LOGGER_NAME_1 );
+logger1 = Logger.getLogger( LOGGER_NAME_1 );
logger1.setLevel(Level.FINE);
-Logger logger2 = Logger.getLogger( LOGGER_NAME_2 );
+logger2 = Logger.getLogger( LOGGER_NAME_2 );
logger2.setLevel(null);
/*
@@ -207,6 +214,18 @@
// verify logger names
ListString loggers1 = mxbean1.getLoggerNames();
ListString loggers2 = mxbean2.getLoggerNames();
+
+// Print the two logger lists for diagnostic purposes
+System.out.println( : LoggingMXBean + loggers1);
+System.out.println( : PlatformLoggingMXBean + loggers2);
+
+// Print the list of logger level comparisons for diagnostic
purposes
+for (String logger : loggers1) {
+System.out.println (: Level ( + logger
++ , + mxbean1.getLoggerLevel(logger)
++ , + mxbean2.getLoggerLevel(logger) + ));
+}
It might be good to keep these diagnostic message to be printed only
when it throws an exception? Extra diagnostic information is good but
just want to keep the volume of traces reasonable not to make the
output harder to use.
We can probably delete the diagnostic output, now that the problem
is clearly identified.
Thanks again for fixing it.
Alan pointed out a pile of teststabilization bugs that have been tagged.
I'll see if any of those were suffering from the same issue found here.
Mandy
Fwd: Re: Miscellaneous minor patches: javadoc typos, javac warnings, etc.
Does anyone have cycles to review and sponsor the Core and SQL clean-ups? They should be trivial to review and push as one change-set, leaving the client area changes for review on the 2d or other list. -Alan. Original Message Subject: Re: Miscellaneous minor patches: javadoc typos, javac warnings, etc. Date: Wed, 09 Nov 2011 11:46:48 +0100 From: Martin Desruisseaux martin.desruisse...@geomatys.fr Organization: Geomatys CC: jdk8-...@openjdk.java.net Hello all It took me a while, but I finally posted the patches that I submitted last month as webrev pages. I tried to split them according different groups (core, Java2D...) to the best of my knowledge: http://webrev.geomatys.com/ I also signed the Oracle Contributor Agreement (OCA) and send it by email to oracle-ca...@oracle.com. In the core classes, most patches can be grouped in two categories: * Documentation fixes (Class, Attributes) * Avoid creation of unnecessary temporary objects (AssertionError, Float, Double) The only real bug fix is in Java 2D (AffineTransform.hashCode() inconsistent with equals(Object) when some coefficients mix positive and negative zeros). However I'm not yet registered on the Java2D mailing list. Should I register on the mailing list of each group for which I may propose a patch? Regards, Martin
Re: Miscellaneous minor patches: javadoc typos, javac warnings, etc.
I looked at the core and sql changes and they are fine. I will great a bug for these and submit the change-set. Best lance On Nov 9, 2011, at 6:30 AM, Alan Bateman wrote: Does anyone have cycles to review and sponsor the Core and SQL clean-ups? They should be trivial to review and push as one change-set, leaving the client area changes for review on the 2d or other list. -Alan. Original Message Subject: Re: Miscellaneous minor patches: javadoc typos, javac warnings, etc. Date: Wed, 09 Nov 2011 11:46:48 +0100 From: Martin Desruisseaux martin.desruisse...@geomatys.fr Organization: Geomatys CC: jdk8-...@openjdk.java.net Hello all It took me a while, but I finally posted the patches that I submitted last month as webrev pages. I tried to split them according different groups (core, Java2D...) to the best of my knowledge: http://webrev.geomatys.com/ I also signed the Oracle Contributor Agreement (OCA) and send it by email to oracle-ca...@oracle.com. In the core classes, most patches can be grouped in two categories: * Documentation fixes (Class, Attributes) * Avoid creation of unnecessary temporary objects (AssertionError, Float, Double) The only real bug fix is in Java 2D (AffineTransform.hashCode() inconsistent with equals(Object) when some coefficients mix positive and negative zeros). However I'm not yet registered on the Java2D mailing list. Should I register on the mailing list of each group for which I may propose a patch? Regards, Martin Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Code Review 7107516: LinkedBlockingQueue/Deque.drainTo(Collection, int) returns 'maxElements' if its value is negative
According to the specification for BlockingQueue.drainTo(Collection c, int maxElements), this method should return the number of elements transferred. However the implementation of this method for LinkedBlockingQueue and LinkedBlockingDeque when given a negative number returns the given negative number. Invoking drainTo(Collection, int) with a value of 0 or less should simply return 0. This change has been pulled from Doug Lea's CVS and I have already reviewed it. Sending to the list for further scrutiny/review. Webrev: http://cr.openjdk.java.net/~chegar/7107516/webrev.00/webrev/ Thanks, -Chris.
Re: Code Review 7107516: LinkedBlockingQueue/Deque.drainTo(Collection, int) returns 'maxElements' if its value is negative
On 09/11/2011 16:44, Mike Duigou wrote: The change looks good. The creation of node instances could use diamond. ie. Yes, this was my initial reaction too. Since Doug's CVS is also built with JDK6 I guess he cannot take advantage of new 7 features. I just tried to keep in sync rather than making a special exception for our downstream copy of this code. I guess going forward we may have to think about how we can use newer features in this area, but I think Doug will have this problem too. -Chris. NodeE node = new NodeE(e); could be : NodeE node = new Node(e); Mike On Nov 9 2011, at 06:55 , Chris Hegarty wrote: According to the specification for BlockingQueue.drainTo(Collection c, int maxElements), this method should return the number of elements transferred. However the implementation of this method for LinkedBlockingQueue and LinkedBlockingDeque when given a negative number returns the given negative number. Invoking drainTo(Collection, int) with a value of 0 or less should simply return 0. This change has been pulled from Doug Lea's CVS and I have already reviewed it. Sending to the list for further scrutiny/review. Webrev: http://cr.openjdk.java.net/~chegar/7107516/webrev.00/webrev/ Thanks, -Chris.
Re: Fwd: Re: Miscellaneous minor patches: javadoc typos, javac warnings, etc.
Martin, Please do register on 2d-dev and propose the 2D changes there. The hashcode change definitely needs discussion, I think there may be views on the NaN comparison as my understanding is that this is supposed to always be not equal. Could be a spec. change for the class if its admissible. Further discussion on this should be on 2d-dev. -phil. PS this is such an unrelated set of changes, I am not sure it should be under one CR, even for 2D. On 11/9/2011 3:30 AM, Alan Bateman wrote: Does anyone have cycles to review and sponsor the Core and SQL clean-ups? They should be trivial to review and push as one change-set, leaving the client area changes for review on the 2d or other list. -Alan. Original Message Subject: Re: Miscellaneous minor patches: javadoc typos, javac warnings, etc. Date: Wed, 09 Nov 2011 11:46:48 +0100 From: Martin Desruisseaux martin.desruisse...@geomatys.fr Organization: Geomatys CC: jdk8-...@openjdk.java.net Hello all It took me a while, but I finally posted the patches that I submitted last month as webrev pages. I tried to split them according different groups (core, Java2D...) to the best of my knowledge: http://webrev.geomatys.com/ I also signed the Oracle Contributor Agreement (OCA) and send it by email to oracle-ca...@oracle.com. In the core classes, most patches can be grouped in two categories: * Documentation fixes (Class, Attributes) * Avoid creation of unnecessary temporary objects (AssertionError, Float, Double) The only real bug fix is in Java 2D (AffineTransform.hashCode() inconsistent with equals(Object) when some coefficients mix positive and negative zeros). However I'm not yet registered on the Java2D mailing list. Should I register on the mailing list of each group for which I may propose a patch? Regards, Martin
Re: Miscellaneous minor patches: javadoc typos, javac warnings, etc.
Hello Phil Le 09/11/11 18:37, Phil Race a écrit : Please do register on 2d-dev and propose the 2D changes there. Registration done, I will post in a few minutes. The hashcode change definitely needs discussion, I think there may be views on the NaN comparison as my understanding is that this is supposed to always be not equal. The proposed change is consistent with the java.lang.Double.equals(Object) behavior. It seems to me the only way to be compliant with the reflexivity contract documented in Object.equals javadoc, apart doing a if (other == this) return true check. Maybe whatever full compliance with Object.equals is strongly desired or not can be a question for the core group? I would like to note that incomplete compliance may be a risk when AffineTransform (or any other object) is used as keys in Hashtable: in current implementation, if an AffineTransform object with at least one NaN value is added in a Hashtable, it is impossible to remove it by a call to Hashtable.remove(Object) (we can still remove it by Iterator.remove()). (Note: my example uses Hashtable instead of HashMap because HashMap has a clever implementation that check for object references before to invoke Object.equals, which invalidate my argument. However not all implementations are that safe). PS this is such an unrelated set of changes, I am not sure it should be under one CR, even for 2D. Actually this is 8 distinct change sets, but webrev merged all my change sets in a single one. Since it is the first time I'm using webrev, I'm probably not using it in the right way. But I still have the 8 distinct changes set on my local Mercurial clone, so I can probably recreate new webrev pages if I learn how to use webrev better... Regards, Martin
Re: Race condition in ThreadGroup stop test
Captured the latest round of comments
- more readable initialization
- allow sleep interruption to terminate main thread
- added current CR# to @bug tag
24/**
25 * @test
26 * @bug 4176355 7084033
27 * @summary Stopping a ThreadGroup that contains the current thread
has
28 * unpredictable results.
29 */
30
31public class Stop implements Runnable {
32private static boolean groupStopped = false ;
33private static final Object lock = new Object();
34
35private static final ThreadGroup group = new ThreadGroup();
36private static final Thread first = new Thread(group, new Stop());
37private static final Thread second = new Thread(group, new
Stop());
38
39public void run() {
40while (true) {
41// Give the other thread a chance to start
42try {
43Thread.sleep(1000);
44} catch (InterruptedException e) {
45}
46
47// When the first thread runs, it will stop the group.
48if (Thread.currentThread() == first) {
49synchronized (lock) {
50try {
51group.stop();
52} finally {
53// Signal the main thread it is time to check
54// that the stopped thread group was
successful
55groupStopped = true;
56lock.notifyAll();
57}
58}
59}
60}
61}
62
63public static void main(String[] args) throws Exception {
64// Launch two threads as part of the same thread group
65second.start();
66first.start();
67
68// Wait for the thread group stop to be issued
69synchronized(lock){
70while (!groupStopped) {
71lock.wait();
72// Give the other thread a chance to stop
73Thread.sleep(1000);
74}
75}
76
77// Check that the second thread is terminated when the
78// first thread terminates the thread group.
79boolean failed = second.isAlive();
80
81// Clean up any threads that may have not been terminated
82first.stop();
83second.stop();
84if (failed)
85throw new RuntimeException(Failure.);
86}
87}
Re: Timing bugs
Here's an update diff for the elapsed time check.
- added current CR# to @bug tag
- moved capture of start time to after creation of the latches
so only the schedule*() and the await() calls are included
in the elapsed time check.
jdk/test/java/util/Timer/Args.java
/*
* @test
- * @bug 6571655 6571881 6574585 6571297
+ * @bug 6571655 6571881 6574585 6571297 6731620
* @summary Test various args to task scheduling methods
*/
@@ -92,19 +92,21 @@
new F(){void f(){ t.scheduleAtFixedRate(x, (Date)null, 42); }}
);
-final long start = System.currentTimeMillis();
-final Date past = new Date(start - 10500);
final CountDownLatch y1 = new CountDownLatch(1);
final CountDownLatch y2 = new CountDownLatch(1);
final CountDownLatch y3 = new CountDownLatch(11);
+final long start = System.currentTimeMillis();
+final Date past = new Date(start - 10500);
+final long elapsed;
schedule( t, counter(y1), past);
schedule( t, counter(y2), past, 1000);
scheduleAtFixedRate(t, counter(y3), past, 1000);
y3.await();
y1.await();
y2.await();
-System.out.printf(elapsed=%d%n, System.currentTimeMillis() - start);
-check(System.currentTimeMillis() - start 500);
+elapsed = System.currentTimeMillis() - start;
+System.out.printf(elapsed=%d%n, elapsed);
+check(elapsed 500);
t.cancel();
On 11/ 4/11 09:36 AM, Gary Adams wrote:
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: Garbage collection race condition before final checks
Here's an updated diff :
- added current CR# to the @bug tag
- made logger1 and logger2 instance variables
- renamed test instance variable lmxbeantest
- removed excessive diagnostic print outs
--- a/test/java/lang/management/PlatformLoggingMXBean/LoggingMXBeanTest.java
+++ b/test/java/lang/management/PlatformLoggingMXBean/LoggingMXBeanTest.java
@@ -23,7 +23,7 @@
/*
* @test
- * @bug 7024172
+ * @bug 7024172 7067691
* @summary Test if proxy for PlatformLoggingMXBean is equivalent
* to proxy for LoggingMXBean
*
@@ -43,6 +43,13 @@
static String LOGGER_NAME_2 = com.sun.management.Logger.Logger2;
static String UNKNOWN_LOGGER_NAME = com.sun.management.Unknown;
+// These instance variables prevent premature logger garbage collection
+// See getLogger() weak reference warnings.
+Logger logger1;
+Logger logger2;
+
+static LoggingMXBeanTest lmxbeantest;
+
public static void main(String[] argv) throws Exception {
MBeanServer mbs = ManagementFactory.getPlatformMBeanServer();
LoggingMXBean proxy =
@@ -51,7 +58,7 @@
LoggingMXBean.class);
// test LoggingMXBean proxy
-LoggingMXBeanTest p = new LoggingMXBeanTest(proxy);
+lmxbeantest = new LoggingMXBeanTest(proxy);
// check if the attributes implemented by PlatformLoggingMXBean
// and LoggingMXBean return the same value
@@ -59,14 +66,16 @@
ManagementFactory.getPlatformMXBean(mbs,
PlatformLoggingMXBean.class);
checkAttributes(proxy, mxbean);
+
+lmxbeantest = null;
}
// same verification as in java/util/logging/LoggingMXBeanTest2
public LoggingMXBeanTest(LoggingMXBean mbean) throws Exception {
-Logger logger1 = Logger.getLogger( LOGGER_NAME_1 );
+logger1 = Logger.getLogger( LOGGER_NAME_1 );
logger1.setLevel(Level.FINE);
-Logger logger2 = Logger.getLogger( LOGGER_NAME_2 );
+logger2 = Logger.getLogger( LOGGER_NAME_2 );
logger2.setLevel(null);
/*
@@ -207,6 +216,7 @@
// verify logger names
ListString loggers1 = mxbean1.getLoggerNames();
ListString loggers2 = mxbean2.getLoggerNames();
+
if (loggers1.size() != loggers2.size())
throw new RuntimeException(LoggerNames: unmatched number of
entries);
ListString loggers3 = new ArrayList(loggers1);
@@ -219,7 +229,10 @@
if (!mxbean1.getLoggerLevel(logger)
.equals(mxbean2.getLoggerLevel(logger)))
throw new RuntimeException(
-LoggerLevel: unmatched level for + logger);
+LoggerLevel: unmatched level for ( + logger
++ , + mxbean1.getLoggerLevel(logger)
++ , + mxbean2.getLoggerLevel(logger) + ));
+
if (!mxbean1.getParentLoggerName(logger)
.equals(mxbean2.getParentLoggerName(logger)))
throw new RuntimeException(
On 11/ 9/11 04:08 AM, Alan Bateman wrote:
On 08/11/2011 15:35, Gary Adams wrote:
Here's another intermittent bug that is attributed
to the garbage collection of the loggers before the
final static check can be applied in the test.
CR#7067691 :
java/lang/management/PlatformLoggingMXBean/LoggingMXBeanTest.java failing
intermittently
I agree with Mandy that it would be great to do a quick audit of the other
logging (and PlatformLoggingMXBean) tests to see if we have similar issues. I
should explain that one reason why these test failures may not have been
observed in the past is because most people ran jtreg in its default mode
(called othervm mode, where each tests runs in its own VM). Nowadays we run
the tests via the make file or use jtreg -agentvm so that tests are running
sequentially in an agent VM. Not all areas can run in agent VM mode yet but
for the areas that do then we get a good speed up as the startup cost is
eliminated and also it's possible to have a pool of agent VMs to make use of
all cores when doing test runs (-agentvm -concurrency:auto for example).
However agent VM makes things less predictable and for these tests it means
that the GC will be unpredictable which is why this test was failing only very
intermittently.
Anyway, the changes look fine to me. I agree with Mandy that many logger1 and
logger2 could be instance variables. I would suggest proxy or something more
readable rather than testp for the LoggingMXBean proxy. I also agree with
Mandy's comment about adding the @bug.
-Alan.
Re: Race condition in TimerTask KillThread test
Here's a revised diff for the KillThread timing problem :
- added current CR# to @bug tag
- capture the thread from the timer task
- wait for the timertask thread to be visible to the main thread
then join the thread before fall through to attempt the second
timertask schedule call.
diff --git a/test/java/util/Timer/KillThread.java
b/test/java/util/Timer/KillThread.java
--- a/test/java/util/Timer/KillThread.java
+++ b/test/java/util/Timer/KillThread.java
@@ -23,7 +23,7 @@
/*
* @test
- * @bug 4288198
+ * @bug 4288198 6818464
* @summary Killing a Timer thread causes the Timer to fail silently on
* subsequent use.
*/
@@ -31,21 +31,27 @@
import java.util.*;
public class KillThread {
+static Thread tdThread;
public static void main (String[] args) throws Exception {
+tdThread = null;
Timer t = new Timer();
// Start a mean event that kills the timer thread
t.schedule(new TimerTask() {
public void run() {
+tdThread = Thread.currentThread();
throw new ThreadDeath();
}
}, 0);
// Wait for mean event to do the deed and thread to die.
try {
+do {
Thread.sleep(100);
+} while(tdThread == null);
} catch(InterruptedException e) {
}
+tdThread.join();
// Try to start another event
try {
On 11/ 6/11 08:46 PM, David Holmes wrote:
On 5/11/2011 8:37 AM, Alan Bateman wrote:
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.
Agreed. As soon as waiting is set we can switch back to the main thread and
proceed to the next phase of the test - but the timer thread is still alive at
that point. If the issue is checking for thread termination then we must track
that directly.
David
-
-Alan.
Re: Garbage collection race condition before final checks
On 09/11/2011 19:26, Gary Adams wrote: Here's an updated diff : - added current CR# to the @bug tag - made logger1 and logger2 instance variables - renamed test instance variable lmxbeantest - removed excessive diagnostic print outs Looks fine to me except that we might find a better name than lmxbeantest, also the setting to null can be removed. One of us needs to push this, listing you as contributor. Mandy or Dan - are you taking it? -Alan.
Re: Garbage collection race condition before final checks
On 11/9/2011 1:18 PM, Alan Bateman wrote: On 09/11/2011 19:26, Gary Adams wrote: Here's an updated diff : - added current CR# to the @bug tag - made logger1 and logger2 instance variables - renamed test instance variable lmxbeantest - removed excessive diagnostic print outs Looks fine to me except that we might find a better name than lmxbeantest, also the setting to null can be removed. One of us needs to push this, listing you as contributor. Mandy or Dan - are you taking it? I will help take this one. Gary, can you also fix PlatformLoggingMXBeanTest.java? We should fix these 2 tests for this bug. Thanks Mandy
Re: Code Review 7107516: LinkedBlockingQueue/Deque.drainTo(Collection, int) returns 'maxElements' if its value is negative
On 10/11/2011 3:03 AM, Chris Hegarty wrote: On 09/11/2011 16:44, Mike Duigou wrote: The change looks good. The creation of node instances could use diamond. ie. Yes, this was my initial reaction too. Since Doug's CVS is also built with JDK6 I guess he cannot take advantage of new 7 features. I just tried to keep in sync rather than making a special exception for our downstream copy of this code. I guess going forward we may have to think about how we can use newer features in this area, but I think Doug will have this problem too. Good point - I was going to make the same comment as Mike. I assume this change was to remove a warning. Changes look good to me. David - -Chris. NodeE node = new NodeE(e); could be : NodeE node = new Node(e); Mike On Nov 9 2011, at 06:55 , Chris Hegarty wrote: According to the specification for BlockingQueue.drainTo(Collection c, int maxElements), this method should return the number of elements transferred. However the implementation of this method for LinkedBlockingQueue and LinkedBlockingDeque when given a negative number returns the given negative number. Invoking drainTo(Collection, int) with a value of 0 or less should simply return 0. This change has been pulled from Doug Lea's CVS and I have already reviewed it. Sending to the list for further scrutiny/review. Webrev: http://cr.openjdk.java.net/~chegar/7107516/webrev.00/webrev/ Thanks, -Chris.
Re: Race condition in ThreadGroup stop test
Gary,
Did you test that this still fails on a JDK without the fix? AFAICS you
must start the threads in the correct order so that in the original bad
code the first thread in the ThreadGroup that would be stopped is
first. Hence
64// Launch two threads as part of the same thread group
65second.start();
66first.start();
needs to be reversed. It is the start() that adds the Thread to the
ThreadGroup's internal array.
David
On 10/11/2011 5:09 AM, Gary Adams wrote:
Captured the latest round of comments
- more readable initialization
- allow sleep interruption to terminate main thread
- added current CR# to @bug tag
24 /**
25 * @test
26 * @bug 4176355 7084033
27 * @summary Stopping a ThreadGroup that contains the current thread has
28 * unpredictable results.
29 */
30
31 public class Stop implements Runnable {
32 private static boolean groupStopped = false ;
33 private static final Object lock = new Object();
34
35 private static final ThreadGroup group = new ThreadGroup();
36 private static final Thread first = new Thread(group, new Stop());
37 private static final Thread second = new Thread(group, new Stop());
38
39 public void run() {
40 while (true) {
41 // Give the other thread a chance to start
42 try {
43 Thread.sleep(1000);
44 } catch (InterruptedException e) {
45 }
46
47 // When the first thread runs, it will stop the group.
48 if (Thread.currentThread() == first) {
49 synchronized (lock) {
50 try {
51 group.stop();
52 } finally {
53 // Signal the main thread it is time to check
54 // that the stopped thread group was successful
55 groupStopped = true;
56 lock.notifyAll();
57 }
58 }
59 }
60 }
61 }
62
63 public static void main(String[] args) throws Exception {
64 // Launch two threads as part of the same thread group
65 second.start();
66 first.start();
67
68 // Wait for the thread group stop to be issued
69 synchronized(lock){
70 while (!groupStopped) {
71 lock.wait();
72 // Give the other thread a chance to stop
73 Thread.sleep(1000);
74 }
75 }
76
77 // Check that the second thread is terminated when the
78 // first thread terminates the thread group.
79 boolean failed = second.isAlive();
80
81 // Clean up any threads that may have not been terminated
82 first.stop();
83 second.stop();
84 if (failed)
85 throw new RuntimeException(Failure.);
86 }
87 }
Re: Race condition in TimerTask KillThread test
Hi Gary,
On 10/11/2011 5:36 AM, Gary Adams wrote:
Here's a revised diff for the KillThread timing problem :
- added current CR# to @bug tag
I don't think that is correct. AFAIK the @bug indicates what bug this
test is testing the fix for, not which bugs modified the test. (Ditto
for your other test fixes).
Alan: can you confirm? Thanks.
public class KillThread {
+ static Thread tdThread;
Must be volatile as its value is being communicated from the timer
thread to the main thread.
public static void main (String[] args) throws Exception {
+ tdThread = null;
Not necessary - static field is null by default.
Timer t = new Timer();
// Start a mean event that kills the timer thread
t.schedule(new TimerTask() {
public void run() {
+ tdThread = Thread.currentThread();
throw new ThreadDeath();
}
}, 0);
// Wait for mean event to do the deed and thread to die.
try {
+ do {
Thread.sleep(100);
+ } while(tdThread == null);
} catch(InterruptedException e) {
}
Minor nit: You didn't introduce this, but catching IE is unnecessary as
main can let it propagate. With the updated code in the unlikely event
it occurred you might then go and invoke join() on a null reference.
Aside: it would be good if whomever is going to do the commits for you
could assist with publishing webrevs for these changes.
Thanks,
David
-
+ tdThread.join();
// Try to start another event
try {
On 11/ 6/11 08:46 PM, David Holmes wrote:
On 5/11/2011 8:37 AM, Alan Bateman wrote:
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.
Agreed. As soon as waiting is set we can switch back to the main
thread and proceed to the next phase of the test - but the timer
thread is still alive at that point. If the issue is checking for
thread termination then we must track that directly.
David
-
-Alan.
Re: Timing bugs
Hi Gary,
Functional changes look okay to me.
Thanks,
David
On 10/11/2011 5:21 AM, Gary Adams wrote:
Here's an update diff for the elapsed time check.
- added current CR# to @bug tag
- moved capture of start time to after creation of the latches
so only the schedule*() and the await() calls are included
in the elapsed time check.
jdk/test/java/util/Timer/Args.java
/*
* @test
- * @bug 6571655 6571881 6574585 6571297
+ * @bug 6571655 6571881 6574585 6571297 6731620
* @summary Test various args to task scheduling methods
*/
@@ -92,19 +92,21 @@
new F(){void f(){ t.scheduleAtFixedRate(x, (Date)null, 42); }}
);
- final long start = System.currentTimeMillis();
- final Date past = new Date(start - 10500);
final CountDownLatch y1 = new CountDownLatch(1);
final CountDownLatch y2 = new CountDownLatch(1);
final CountDownLatch y3 = new CountDownLatch(11);
+ final long start = System.currentTimeMillis();
+ final Date past = new Date(start - 10500);
+ final long elapsed;
schedule( t, counter(y1), past);
schedule( t, counter(y2), past, 1000);
scheduleAtFixedRate(t, counter(y3), past, 1000);
y3.await();
y1.await();
y2.await();
- System.out.printf(elapsed=%d%n, System.currentTimeMillis() - start);
- check(System.currentTimeMillis() - start 500);
+ elapsed = System.currentTimeMillis() - start;
+ System.out.printf(elapsed=%d%n, elapsed);
+ check(elapsed 500);
t.cancel();
On 11/ 4/11 09:36 AM, Gary Adams wrote:
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
