Re: RFR: 8143121: javax/management/remote/mandatory/loading/MethodResultTest.java fails intermittently

[Shanliang JIANG]

We could repeat retrying until the test harness timeout, but add a sleep is 
definitively useful between retrying.

Shanliang
> On 23 Nov 2015, at 12:32, Daniel Fuchs  wrote:
> 
> Hi Alexander,
> 
> This looks a bit dangerous to me - it could create a busy loop
> if for some reason the connection can never go through.
> 
> I would suggest retrying only once (or retrying a fixed number
> of times) - and possibly introducing a small delay (Thread.sleep)
> before retrying.
> 
> best regards,
> 
> -- daniel
> 
> On 23/11/15 12:19, Alexander Kulyakhtin wrote:
>> Hi,
>> 
>> Could you, please, review this small, test-only, change:
>> 
>> CR: https://bugs.openjdk.java.net/browse/JDK-8143121
>> Webrev: 
>> http://cr.openjdk.java.net/~akulyakh/8143121/test/javax/management/remote/mandatory/loading/MethodResultTest.java.udiff.html
>> 
>> The precondition for this test is a JMXConnector having established a 
>> successful connection.
>> On some environments the test sometimes can not connect at the first 
>> attempt, because the target application is not yet ready.
>> We are changing the test so that it tries to connect again if it gets 
>> IOException during the connection.
>> 
>> Best regards,
>> Alexander
>> 
> 

Re: jmx-dev RFR 8141591: javax/management/remote/mandatory/threads/ExecutorTest.java fails intermittently

[Shanliang JIANG]

> On 13 Nov 2015, at 09:04, Jaroslav Bachorik  
> wrote:
> 
> On 13.11.2015 08:05, Shanliang JIANG wrote:
>> Hi Jaroslav,
>> 
>> The issue is that after a JMX client is terminated, its
>> ClientNotifForwarder continues deliver a job to a user specific
>> Executor, I think a better fix to not allow this happen.
>> 
>> I am not sure that it is a good solution to check
>> RejectedExecutionException, here is the Java doc:
>> "Exception thrown by an |Executor|
>> 
>>  when
>> a task cannot be accepted for execution."
>> 
>> it means that  the exception is possibly thrown in other cases too, like
>> too many tasks if it is shared. So ignore simply this exception in case
>> of not “shouldStop()” seems incorrect.
>> 
>> And Executor is an interface and a user could provide any implementation
>> class, so possible a user would throw any another RuntimeException or
>> even an Error in this case.
> Well, the main problem is the self-rescheduling part. Normally, a scheduled 
> executor would be used to perform periodic tasks and it would know how to 
> handle its own shutdown. But, unfortunately, the more generic Executor is 
> required. If only it were at least ExecutorService where you can use 
> 'isShutdown()' method ...
> 
> The fact that the user provided executor can throw RejectedExecutionException 
> at its discretion opens whole another can of worms. The code as it is now 
> will simply bail out of the notification fetching loop with the thrown 
> exception. Sadly, there is no cleanup performed so the ClientNotifForwarder 
> will stay in inconsistent state (marked as STARTED when it is, in fact, 
> TERMINATED, notification listeners not de-registered, etc.). To make things 
> worse there seem to be no official documentation as what is the expected 
> behaviour of the externally provided executor. The only documentation to the 
> env property "jmx.remote.x.fetch.notifications.executor" is in 
> ClientNotifForwarder.java (L125-128) and it is not exactly exhaustive.

Here is my suggestion (I create a webrev because it is easier to look at):
http://cr.openjdk.java.net/~sjiang/JDK-8141591/00/ 
<http://cr.openjdk.java.net/~sjiang/JDK-8141591/00/>

It does rescheduling within the synchronisation, which makes sure to not 
deliver a new task after a JMX client is terminated. 

This class is complicated because the fetching should be stopped if no more 
listener or during re-connection, but then could be re-started at anytime.

Shanliang


> 
> -JB-
> 
> 
>> 
>> Shanliang
>> > On 12 Nov 2015, at 13:13, Jaroslav Bachorik
>> > mailto:jaroslav.bacho...@oracle.com>>
>> > wrote:
>> >
>> > Please, review the following test change
>> >
>> > Issue : https://bugs.openjdk.java.net/browse/JDK-8141591
>> > Webrev: http://cr.openjdk.java.net/~jbachorik/8141591/webrev.00
>> >
>> > In rare circumstances, when an external executor is provided, the
>> > ClientNotifForwarder$NotifFetcher.doRun() method might fail because of
>> > RejectedExecutionException caused by the executor being externally
>> > shut down.
>> >
>> > The patch adds a guard against this possibility. If the executor has
>> > been shut down the fetcher will also properly stop.
>> >
>> > Thanks,
>> >
>> > -JB-
>> 
> 

Re: RFR: 8141591 : javax/management/remote/mandatory/threads/ExecutorTest.java fails intermittently

[Shanliang JIANG]

When the executor is shutdown, the client and server were closed, so the test 
should not receive any exception.

The better solution is fix in com.sun.jmx.remote.internal.ClientNotifForwarder 
Line 829:
  synchronized (ClientNotifForwarder.this) { 
 if (state == STARTED) { 
executor.execute(new NotifFetcher()); 
 } 
  }

Thanks to work on this issue.
Shanliang

> On 10 Nov 2015, at 12:36, Alexander Kulyakhtin 
>  wrote:
> 
> Hi Jaroslav, Shanliang Jiang
> 
> Thank you very much for the review.
> 
> Following the comments from Shanliang Jiang on 
> https://bugs.openjdk.java.net/browse/JDK-8141591 could it be possible to 
> make, instead, a fix in the jdk source and leave the test unchanged
> 
> as per the following webrev : 
> http://cr.openjdk.java.net/~akulyakh/814591jdk/src/java.management/share/classes/com/sun/jmx/remote/internal/ClientNotifForwarder.java.udiff.html
> 
> If these changes are problematic I will go for the test changes which you 
> have already accepted.
> 
> Best regards,
> Alexander
> 
> - Original Message -
> From: jaroslav.bacho...@oracle.com
> To: alexander.kulyakh...@oracle.com, serviceability-dev@openjdk.java.net
> Cc: marti...@google.com
> Sent: Tuesday, November 10, 2015 2:10:48 PM GMT +03:00 Iraq
> Subject: Re: RFR: 8141591 : 
> javax/management/remote/mandatory/threads/ExecutorTest.java fails 
> intermittently
> 
> On 9.11.2015 19:07, Alexander Kulyakhtin wrote:
>> Hi,
>> 
>> Following the comments from Jaroslav  and Martin I've changed the test
>> to use the unbound CountDownLatch.await()
>> 
>> Since await() will wait undefinitely and thus the test, if fails, will
>> now fail by timeout only the code has been additionally simplified to
>> reflect that.
>> 
>> Please, find the updated webrev at:
>> 
>> CR: https://bugs.openjdk.java.net/browse/JDK-8141591
>> RFR:
>> http://cr.openjdk.java.net/~akulyakh/8141591_03/test/javax/management/remote/mandatory/threads/ExecutorTest.java.udiff.html
> 
> Looks good!
> 
> -JB-
> 
>> 
>> Best regards,
>> Alexander
>> 
>> - Original Message -
>> From: marti...@google.com
>> To: jaroslav.bacho...@oracle.com
>> Cc: alexander.kulyakh...@oracle.com, serviceability-dev@openjdk.java.net
>> Sent: Monday, November 9, 2015 7:53:08 PM GMT +03:00 Iraq
>> Subject: Re: RFR: 8141591 :
>> javax/management/remote/mandatory/threads/ExecutorTest.java fails
>> intermittently
>> 
>> The traditional way is to have all the worker tasks count down a latch
>> when they're started and have the master thread wait on that before
>> proceeding.
>> 
>> You can use test.timeout.factor to do a scaled timed wait.
>> 
>> On Mon, Nov 9, 2015 at 8:03 AM, Jaroslav Bachorik
>> mailto:jaroslav.bacho...@oracle.com>> wrote:
>> 
>>Hi Alexander,
>> 
>>On 9.11.2015 16:40, Alexander Kulyakhtin wrote:
>> 
>>Hi,
>> 
>>Could you, please, review this test-only fix:
>> 
>>CR: https://bugs.openjdk.java.net/browse/JDK-8141591
>>Webrev:
>>
>> http://cr.openjdk.java.net/~akulyakh/8141591_01/test/javax/management/remote/mandatory/threads/ExecutorTest.java.udiff.html
>> 
>>Before the fix, it was possible that we shut down the executor
>>before all the jobs have been submitted. This resulted in
>>RejectedExecutionException.
>>We are modifying the test to wait on CountDownLatch untill all
>>the jobs have completed their execute()
>> 
>> 
>>On L175 you should be using the unbound version of await(). It would
>>be better to let the harness deal with the timeout.
>> 
>>Otherwise it looks good.
>> 
>>Cheers,
>> 
>>-JB-
>> 
>> 
>>Best regards,
>>Alexander
>> 
>> 
>> 
> 

Re: RFR (xs) 8132003: Update javax/management regression test for Verona (versioning)

[Shanliang Jiang]

Iris,

The fix ooks good.

Shanliang

Iris Clark wrote:

Hi.

Please review changes to resolve the following bug:

8132003: Update javax/management regression test for Verona (versioning)
Bug: https://bugs.openjdk.java.net/browse/JDK-8132003

The regression test javax/management/remote/mandatory/notif/NotSerializableNotifTest.java 
fails in Verona [0,1] builds because it assumes that the system property java.verison 
will always begin with "1.".  Verona drops this part of the version string for 
JDK 9 and later.

The test attempts to discern whether it is being run against JDK 1.4 or 
earlier.  Since I believe that this test will never be run against JDK 1.4, I 
expect that we can entirely remove the conditional.  Here's the required diff:

--- a/test/javax/management/remote/mandatory/notif/NotSerializableNotifTest.java
Mon Jul 20 11:01:24 2015 -0700
+++ b/test/javax/management/remote/mandatory/notif/NotSerializableNotifTest.java
Mon Jul 20 19:31:54 2015 -0700
@@ -24,7 +24,7 @@
 /*
  * @test
  * @summary Tests to send a not serializable notification.
- * @bug 5022196
+ * @bug 5022196 8132003
  * @author Shanliang JIANG
  * @modules java.management
  * @run clean NotSerializableNotifTest
@@ -53,22 +53,13 @@
 private static final MBeanServer mbeanServer = 
MBeanServerFactory.createMBeanServer();
 private static ObjectName emitter;

-private static String[] protocols;
+private static String[] protocols = new String[] {"rmi", "iiop", "jmxmp"};

 private static final int sentNotifs = 10;

 public static void main(String[] args) throws Exception {
 System.out.println(">>> Test to send a not serializable notification");

-// IIOP fails on JDK1.4, see 5034318
-final String v = System.getProperty("java.version");
-float f = Float.parseFloat(v.substring(0, 3));
-if (f<1.5) {
-protocols = new String[] {"rmi", "jmxmp"};
-} else {
-protocols = new String[] {"rmi", "iiop", "jmxmp"};
-}
-
 emitter = new ObjectName("Default:name=NotificationEmitter");
 mbeanServer.registerMBean(new NotificationEmitter(), emitter);

If you believe that we need to keep the conditional, I can submit an alternate 
diff.  Let me know what you'd prefer.  Please also let me know if there are any 
JMX-specific development processes I should be aware of (e.g. required number 
of Reviewers, testing requirements, etc.).

After review, the changeset will be pushed to verona/stage [2]. The changeset 
will go to jdk9/* when Verona is complete later this summer.

Thanks,
iris

[0] http://openjdk.java.net/projects/verona
[1] http://openjdk.java.net/jeps/223
[2] http://hg.openjdk.java.net/verona/stage
  

Re: RFR 8029098: Exclude javax/management/remote/mandatory/notif/ListenerScaleTest.java from running on fastdebug builds

[Shanliang Jiang]

Looks good to me!

Shanliang


Jaroslav Bachorik wrote:

Please, review the following simple change

Issue : https://bugs.openjdk.java.net/browse/JDK-8029098
Webrev: http://cr.openjdk.java.net/~jbachorik/8029098/webrev.00

The 'ListenerScaleTest' is meant to check the proper scaling of the 
MBean notification dispatching. This doesn't make much sense for 
fastdebug builds where all the timing is rather skewed.


The patch checks for the fastdebug build and just skips the test and 
outputs a notification message.


Thanks,

-JB-

Re: Potential infinite waiting at JMXConnection#createConnection

[Shanliang Jiang]

KUBOTA Yuji wrote:

Hi Shanliang,

Many thanks for your help! 

I do not have any role yet. So I can not create a new bug at JBS. It's 
a reason why I submitted a mail with my patch at first.


This issue is caused by a rare network problem during the flush() [3] 
. I got this infinite loop only once. So I will try to write test 
or/and client codes with BCI for reproduction.
To reproduce the bug, I was thinking to use 
RMI_SERVER_SOCKET_FACTORY_ATTRIBUTE to specify a user socket server, 
which will not response any client connection request, I did not yet 
test this solution.


Shanliang


Thanks,
Yuji

2015-05-06 18:51 GMT+09:00 Shanliang Jiang <mailto:shanliang.ji...@oracle.com>>:


Hi Yuji,

I think better at first to create a bug at:
https://bugs.openjdk.java.net/secure/Dashboard.jspa

It looks like an issue for me, it must be possible to have a test
to reproduce the issue. It is helpful to attach the test and
present your solution in the bug.

I can help if you need any help to create the bug.

Shanliang



KUBOTA Yuji wrote:

My apologies for re-post, I forgot to register serviceability-dev
before the last post.

    Hi Shanliang,

Thanks you for your help!

RMI_CLIENT_SOCKET_FACTORY_ATTRIBUTE is a nice workaround.

However, many users believe sun.rmi.transport.tcp.responseTimeout
to specify the timeout, 
e.g. the second flush() of TCPChannel#createConnection [2]. 
In really, the first flush() [3] is not affected by
sun.rmi.transport.tcp.responseTimeout, 
and will be the (potential) infinite waiting by bad luck. So I

think openjdk should fix it for users.

[2]:

http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/c5b5d9045728/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java#l296
[3]:

http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/c5b5d9045728/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java#l227

Thanks,
Yuji

2015-05-05 2:03 GMT+09:00 Shanliang Jiang
mailto:shanliang.ji...@oracle.com>>:
> Hi Yuji,
>
> (I reply to serviceability alias)
>
> When you create a RMI server connector, you can specify a
> RMIClientSocketFactory by RMI_CLIENT_SOCKET_FACTORY_ATTRIBUTE,
this allows
> you to specify your SoTimeout.
>
> Hope this helps.
>
> Shanliang
>
>
>
> KUBOTA Yuji wrote:
>
> Hi all,
>
> I want to contribute this issue.
> If there are a problem about this patch or a better way for openjdk
> community, please advise me.
>
> Thanks for
>
> 2015-04-22 0:31 GMT+09:00 KUBOTA Yuji mailto:kubota.y...@gmail.com>>:
>  
>

> Hi all,
>
> I found an infinite waiting at TCPChannel#createConnection.
> This method flushes the DataOutputStream without the socket
timeout settings
> when choose stream protocol [1].
>
> If connection lost (the destination server do no return response)
> during the flush,
> this method has possibilities to take long time beyond the
expectations
> at java.net.SocketInputStream.socketRead0 as following stack trace.
>
> stack trace :
> at
java.net.SocketInputStream.socketRead0(SocketInputStream.java)
> at java.net.SocketInputStream.read(SocketInputStream.java)
> at java.net.SocketInputStream.read(SocketInputStream.java)
> at sun.security.ssl.InputRecord.readFully(InputRecord.java)
> at sun.security.ssl.InputRecord.read(InputRecord.java)
> at
sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java)
> at
>
sun.security.ssl.SSLSocketImpl.performInitialHandshake(SSLSocketImpl.java)
> at
sun.security.ssl.SSLSocketImpl.writeRecord(SSLSocketImpl.java)
> at
sun.security.ssl.AppOutputStream.write(AppOutputStream.java)
> at
> java.io.BufferedOutputStream.flushBuffer(BufferedOutputStream.java)
> at
java.io.BufferedOutputStream.flush(BufferedOutputStream.java)
> at java.io.DataOutputStream.flush(DataOutputStream.java)
> at
> sun.rmi.transport.tcp.TCPChannel.createConnection(TCPChannel.java)
> at
sun.rmi.transport.tcp.TCPChannel.newConnection(TCPChannel.java)
> at sun.rmi.server.UnicastRef.invoke(UnicastRef.java)
> at javax.management.remote.rmi.RMIServerImpl_Stub.newClient
> at
>
javax.management.remote.rmi.RMIConnector.getConnection(RMIConnector.java)
> at
> javax.management.remote.rmi.RMIConnector.connect(RMIConnector.java)
> at
>

javax.management.remote.JMXConnectorFactory.connect(JMXConnec

Re: Potential infinite waiting at JMXConnection#createConnection

[Shanliang Jiang]

Hi Yuji,

I think better at first to create a bug at:
   https://bugs.openjdk.java.net/secure/Dashboard.jspa

It looks like an issue for me, it must be possible to have a test to 
reproduce the issue. It is helpful to attach the test and present your 
solution in the bug.


I can help if you need any help to create the bug.

Shanliang


KUBOTA Yuji wrote:
My apologies for re-post, I forgot to register serviceability-dev 
before the last post.


Hi Shanliang,

Thanks you for your help!

RMI_CLIENT_SOCKET_FACTORY_ATTRIBUTE is a nice workaround.

However, many users believe sun.rmi.transport.tcp.responseTimeout 
to specify the timeout, 
e.g. the second flush() of TCPChannel#createConnection [2]. 
In really, the first flush() [3] is not affected by 
sun.rmi.transport.tcp.responseTimeout, 
and will be the (potential) infinite waiting by bad luck. So I think 
openjdk should fix it for users.


[2]: 
http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/c5b5d9045728/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java#l296
[3]: 
http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/c5b5d9045728/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java#l227


Thanks,
Yuji

2015-05-05 2:03 GMT+09:00 Shanliang Jiang <mailto:shanliang.ji...@oracle.com>>:

> Hi Yuji,
>
> (I reply to serviceability alias)
>
> When you create a RMI server connector, you can specify a
> RMIClientSocketFactory by RMI_CLIENT_SOCKET_FACTORY_ATTRIBUTE, this 
allows

> you to specify your SoTimeout.
>
> Hope this helps.
>
> Shanliang
>
>
>
> KUBOTA Yuji wrote:
>
> Hi all,
>
> I want to contribute this issue.
> If there are a problem about this patch or a better way for openjdk
> community, please advise me.
>
> Thanks for
>
> 2015-04-22 0:31 GMT+09:00 KUBOTA Yuji <mailto:kubota.y...@gmail.com>>:
>  
>

> Hi all,
>
> I found an infinite waiting at TCPChannel#createConnection.
> This method flushes the DataOutputStream without the socket timeout 
settings

> when choose stream protocol [1].
>
> If connection lost (the destination server do no return response)
> during the flush,
> this method has possibilities to take long time beyond the expectations
> at java.net.SocketInputStream.socketRead0 as following stack trace.
>
> stack trace :
> at 
java.net.SocketInputStream.socketRead0(SocketInputStream.java)

> at java.net.SocketInputStream.read(SocketInputStream.java)
> at java.net.SocketInputStream.read(SocketInputStream.java)
> at sun.security.ssl.InputRecord.readFully(InputRecord.java)
> at sun.security.ssl.InputRecord.read(InputRecord.java)
> at sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java)
> at
> 
sun.security.ssl.SSLSocketImpl.performInitialHandshake(SSLSocketImpl.java)
> at 
sun.security.ssl.SSLSocketImpl.writeRecord(SSLSocketImpl.java)

> at sun.security.ssl.AppOutputStream.write(AppOutputStream.java)
> at
> java.io.BufferedOutputStream.flushBuffer(BufferedOutputStream.java)
> at java.io.BufferedOutputStream.flush(BufferedOutputStream.java)
> at java.io.DataOutputStream.flush(DataOutputStream.java)
> at
> sun.rmi.transport.tcp.TCPChannel.createConnection(TCPChannel.java)
> at 
sun.rmi.transport.tcp.TCPChannel.newConnection(TCPChannel.java)

> at sun.rmi.server.UnicastRef.invoke(UnicastRef.java)
> at javax.management.remote.rmi.RMIServerImpl_Stub.newClient
> at
> 
javax.management.remote.rmi.RMIConnector.getConnection(RMIConnector.java)

> at
> javax.management.remote.rmi.RMIConnector.connect(RMIConnector.java)
> at
> 
javax.management.remote.JMXConnectorFactory.connect(JMXConnectorFactory.java)

>
> When create connection, we cannot set the timeout by properties.
> Therefore, JMX sets the default value of SO_TIMEOUT, i.e., infinite.
> So I wrote a patch to fix this infinite waiting by using 
property-configured

> value:
> sun.rmi.transport.tcp.responseTimeout.
>
> Please review this patch. :)
>
> Note: My OCA has been processed a few hour ago, so my name may take a
> short time to
> appear on the OCA signatories page.
>
> Thanks,
> KUBOTA Yuji
>
> [1]:
> 
http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/c5b5d9045728/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPConnection.java#l191

>
> diff --git
> a/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
> b/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
> --- a/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
> +++ b/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
> @@ -222,20 +222,34 @@
>  // choose protocol (single op if not reusable socket)
>   

Re: Potential infinite waiting at JMXConnection#createConnection

[Shanliang Jiang]

Hi Yuji,

(I reply to serviceability alias)

When you create a RMI server connector, you can specify a 
RMIClientSocketFactory by RMI_CLIENT_SOCKET_FACTORY_ATTRIBUTE 
<http://docs.oracle.com/javase/7/docs/api/javax/management/remote/rmi/RMIConnectorServer.html#RMI_CLIENT_SOCKET_FACTORY_ATTRIBUTE>, 
this allows you to specify your SoTimeout.


Hope this helps.

Shanliang


KUBOTA Yuji wrote:

Hi all,

I want to contribute this issue.
If there are a problem about this patch or a better way for openjdk
community, please advise me.

Thanks for

2015-04-22 0:31 GMT+09:00 KUBOTA Yuji :
  

Hi all,

I found an infinite waiting at TCPChannel#createConnection.
This method flushes the DataOutputStream without the socket timeout settings
when choose stream protocol [1].

If connection lost (the destination server do no return response)
during the flush,
this method has possibilities to take long time beyond the expectations
at java.net.SocketInputStream.socketRead0 as following stack trace.

stack trace :
at java.net.SocketInputStream.socketRead0(SocketInputStream.java)
at java.net.SocketInputStream.read(SocketInputStream.java)
at java.net.SocketInputStream.read(SocketInputStream.java)
at sun.security.ssl.InputRecord.readFully(InputRecord.java)
at sun.security.ssl.InputRecord.read(InputRecord.java)
at sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java)
at 
sun.security.ssl.SSLSocketImpl.performInitialHandshake(SSLSocketImpl.java)
at sun.security.ssl.SSLSocketImpl.writeRecord(SSLSocketImpl.java)
at sun.security.ssl.AppOutputStream.write(AppOutputStream.java)
at java.io.BufferedOutputStream.flushBuffer(BufferedOutputStream.java)
at java.io.BufferedOutputStream.flush(BufferedOutputStream.java)
at java.io.DataOutputStream.flush(DataOutputStream.java)
at sun.rmi.transport.tcp.TCPChannel.createConnection(TCPChannel.java)
at sun.rmi.transport.tcp.TCPChannel.newConnection(TCPChannel.java)
at sun.rmi.server.UnicastRef.invoke(UnicastRef.java)
at javax.management.remote.rmi.RMIServerImpl_Stub.newClient
at 
javax.management.remote.rmi.RMIConnector.getConnection(RMIConnector.java)
at javax.management.remote.rmi.RMIConnector.connect(RMIConnector.java)
at 
javax.management.remote.JMXConnectorFactory.connect(JMXConnectorFactory.java)

When create connection, we cannot set the timeout by properties.
Therefore, JMX sets the default value of SO_TIMEOUT, i.e., infinite.
So I wrote a patch to fix this infinite waiting by using property-configured 
value:
sun.rmi.transport.tcp.responseTimeout.

Please review this patch. :)

Note: My OCA has been processed a few hour ago, so my name may take a
short time to
appear on the OCA signatories page.

Thanks,
KUBOTA Yuji

[1]: 
http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/c5b5d9045728/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPConnection.java#l191

diff --git a/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
b/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
--- a/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
+++ b/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
@@ -222,20 +222,34 @@
 // choose protocol (single op if not reusable socket)
 if (!conn.isReusable()) {
 out.writeByte(TransportConstants.SingleOpProtocol);
 } else {
 out.writeByte(TransportConstants.StreamProtocol);
+
+int usableSoTimeout = 0;
+try {
+/*
+ * If socket factory had set a zero timeout on its own,
+ * then set the property-configured value to prevent
+ * an infinite waiting.
+ */
+usableSoTimeout = sock.getSoTimeout();
+if (usableSoTimeout == 0) {
+  usableSoTimeout = responseTimeout;
+}
+sock.setSoTimeout(usableSoTimeout);
+} catch (Exception e) {
+// if we fail to set this, ignore and proceed anyway
+}
 out.flush();

 /*
  * Set socket read timeout to configured value for JRMP
  * connection handshake; this also serves to guard against
  * non-JRMP servers that do not respond (see 4322806).
  */
-int originalSoTimeout = 0;
 try {
-originalSoTimeout = sock.getSoTimeout();
 sock.setSoTimeout(handshakeTimeout);
 } catch (Exception e) {
 // if we fail to set this, ignore and proceed 

Re: RFR: 8078144 many nightly tests failed due to NoSuchMethodError: sun.management.ManagementFactoryHelper.getDiagnosticMXBean

[Shanliang Jiang]

Alan Bateman wrote:

On 21/04/2015 08:01, Shanliang Jiang wrote:

Hi,

Please review this test fix:

webrev: http://cr.openjdk.java.net/~sjiang/JDK-8078144/00/
bug: https://bugs.openjdk.java.net/browse/JDK-8078144

The method sun.management.ManagementFactoryHelper.getDiagnosticMXBean 
was removed because HotSpotDiagnosticMXBean is not in java.management 
module but in jdk.management module, to get the 
HotSpotDiagnosticMXBean we should call:

ManagementFactory.getPlatformMXBean(HotSpotDiagnosticMXBean.class)

I passed all failed tests with the fix.

I see JInfoRunningProcessFlagTest still imports 
sun.management.ManagementFactoryHelper. Best to remove that now as 
otherwise this issue will come back again once javac has support for 
modules. Otherwise looks okay to me.

OK I have removed that useless import, here is the new version:
   http://cr.openjdk.java.net/~sjiang/JDK-8078144/01/

by the way the test JInfoRunningProcessFlagTest is in 
jdk/test/ProblemList.txt and I found the issue by "find ./ -name 
"*.java" |xargs grep ManagementFactoryHelper.getDiagnosticMXBean ". So 
my fix just makes it compilable but it still fails


Thanks,
Shanliang

RFR: 8078144 many nightly tests failed due to NoSuchMethodError: sun.management.ManagementFactoryHelper.getDiagnosticMXBean

[Shanliang Jiang]

Hi,

Please review this test fix:

webrev: http://cr.openjdk.java.net/~sjiang/JDK-8078144/00/
bug: https://bugs.openjdk.java.net/browse/JDK-8078144

The method sun.management.ManagementFactoryHelper.getDiagnosticMXBean 
was removed because HotSpotDiagnosticMXBean is not in java.management 
module but in jdk.management module, to get the HotSpotDiagnosticMXBean 
we should call:

ManagementFactory.getPlatformMXBean(HotSpotDiagnosticMXBean.class)

I passed all failed tests with the fix.

Thanks,
Shanliang

RFR: 8077408 javax/management/remote/mandatory/notif/NotSerializableNotifTest.java fails due to Port already in use: 2468

[Shanliang Jiang]

Hi,

Please review this test fix.

Instead to specify a port, we use "0" to get a free port.

I also remove the test waiting time variable, better to reply on 
directly the test harness timeout.


bug: https://bugs.openjdk.java.net/browse/JDK-8077408
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8077408/00/


Thanks,
Shanliang

Re: RFR 8042901: Allow com.sun.management to be in a different module to java.lang.management

[Shanliang Jiang]

Mandy Chung wrote:

CheckSomeMXBeanImplPackage.java
  line 45-50 & 58-60: should be called unconditionally since they
  should pass if java.management is present.
The method "check" checks that an MBean implementation must be from 
"com.sun.management.internal", so even we look for an MXBean with its 
standard MXBean class (java.lang.management.*), do "check" still needs 
jdk.management module to be present.


We need more unit tests for the case when jdk.management is absent.

Thanks,
Shanliang

Re: RFR 8042901: Allow com.sun.management to be in a different module to java.lang.management

[Shanliang Jiang]

Hi,

I have to ask the review again because I need to modify:
   langtools/src/jdk.dev/share/classes/com/sun/tools/jdeps/Profile.java

The issue was found when langtools tests were added into my test list.

The new version is:
   http://cr.openjdk.java.net/~sjiang/JDK-8042901/02/

which integrated also the Mandy's comments in the following mail.

Thanks,
Shanliang

Mandy Chung wrote:

On 3/31/2015 9:39 AM, shanliang wrote:

Please review this fix:

Bug: https://bugs.openjdk.java.net/browse/JDK-8042901
Webrev: http://cr.openjdk.java.net/~sjiang/JDK-8042901/00/


Thank you for doing this big change to separate com.sun.management
API from java.management module.   Looking really good.

Also thanks for fixing the tests to eliminate the unnecessary use of
JDK internal APIs.

modules.xml change looks good to me.

sun/management/ThreadImpl.java
  35 /**
  36  * Implementation class for the thread subsystem.
  37  * Standard and committed hotspot-specific metrics if any.
  38  *
  39  * ManagementFactory.getThreadMXBean() returns an instance
  40  * of this class.
  41  */
  42 // the implementation for com.sun.management.ThreadMXBean can
  43 // be moved to jdk.management in the future.

What about:
  Implementation for java.lang.management.ThreadMXBean as well as
  providing the supporting method for com.sun.management.ThreadMXBean.
The supporting method for com.sun.management.ThreadMXBean can
  be moved to jdk.management in the future.

CheckSomeMXBeanImplPackage.java
   Thanks for adding this test.

   Iterating jrt file system to find jdk.management module is one way.
   Another simpler alternative is to call:
 Class.forName("com.sun.management.GarbageCollectorMXBean");
   and catch CNFE to determine if it's present or not.

   You should also call ManagementFactory.getPlatformMXBeans on
   com.sun.management.GarbageCollectorMXBean if present.

VMOptionOpenDataTest.java
   copyright header year is wrong.

PlatformMBeanProviderConstructorCheck.java
   The test needs to restore the original policy and security manager
   before the test exits in case this case runs in agent vm mode.
   The static permName and accepted variables are more appropriate
   in MyPolicy class.  Perhaps rename "accepted" to an instance
   "denied" or "allowed" variable of MyPolicy class to reflect
   what it intends to mean.

I'm okay if you make the change before the push.  No need for a new
webrev.

Mandy

Re: RFR 8042901: Allow com.sun.management to be in a different module to java.lang.management

[shanliang]

Erik Joelsson wrote:

Hello,

(Adding build-dev since this touches makefiles and jigsaw-dev since 
this touches modules.xml)


In general, the build changes look pretty good. I much appreciate 
moving the OS specific source into OS specific source dirs. A few 
notes however. Though I realize you followed existing patterns, we 
have some more current best practices that I would like to incorporate 
in new code.


jdk/make/lib/Lib-jdk.management.gmk:

The variables BUILD_LIBJDKMANAGEMENT_SRC and 
BUILD_LIBJDKMANAGEMENT_CFLAGS should lose the "BUILD_" prefix. While 
it works, it makes them unnecessarily long and it risks conflicting 
with internal variables created in the SetupNativeCompilation call.

Done.


BUILD_LIBJDKMANAGEMENT_EXCLUDES is unused and should just be removed. 
The EXCLUDE_FILES parameter too.

Done.


LIBJDKMANAGEMENT_MAPFILE should be removed. This was a special 
construct for libmanagement used for a while until cmm was split into 
a separate module. Just inline the mapfile line into the macro call.

Done.



jdk/make/lib/Lib-java.management.gmk:

BUILD_LIBMANAGEMENT_EXCLUDES is unused here as well.

Removed.


LIBMANAGEMENT_MAPFILE should be inlined here as well.

Done.


While you are at it, might as well fix the BUILD_ prefix on the SRC 
and CFLAGS variables here too if you don't mind.

Done.



Is the need for low optimization when debug symbols are active still 
valid for both libmanagement and libmanagement_ext?
Sorry I do not know, I did not touch that flag stting in libmanagement, 
and I copied it for libmanagement_ext.


Here is the new version:
   http://cr.openjdk.java.net/~sjiang/JDK-8042901/01/

Thanks for the review.
Shanliang



/Erik

On 2015-03-31 18:39, shanliang wrote:

Please review this fix:

Bug: https://bugs.openjdk.java.net/browse/JDK-8042901
Webrev: http://cr.openjdk.java.net/~sjiang/JDK-8042901/00/

Some code within the module java.management is separated and moved to 
the new module jdk.management, the new module takes the 
implementation code for Oracle Corporation's platform extension to 
the implementation of the java.lang.management API and also the 
management interface for some other components for the platform.


Thanks,
Shanliang

Re: RFR 8042901: Allow com.sun.management to be in a different module to java.lang.management

[shanliang]

Jaroslav Bachorik wrote:

Hi Shanliang,

Looks good! A lot of work here ...

Just a nit -

* 
jdk/src/jdk.management/share/classes/com/sun/management/internal/DiagnosticCommandInfo.java 


  - superfluous import @L28 (and the copyright year change)

Yes the import of L28 is unnecessary.
The copyright year was changed because the package was changed.


I suppose all reg-tests and related JCK tests are still passing.

I passed svc/core/hotspot unit tests with jprt, and JMX JCK tests.

Thanks for the review.
Shanliang


-JB-

On 31.3.2015 18:39, shanliang wrote:

Please review this fix:

Bug: https://bugs.openjdk.java.net/browse/JDK-8042901
Webrev: http://cr.openjdk.java.net/~sjiang/JDK-8042901/00/

Some code within the module java.management is separated and moved to
the new module jdk.management, the new module takes the implementation
code for Oracle Corporation's platform extension to the implementation
of the java.lang.management API and also the management interface for
some other components for the platform.

Thanks,
Shanliang

RFR 8042901: Allow com.sun.management to be in a different module to java.lang.management

[shanliang]

Please review this fix:

Bug: https://bugs.openjdk.java.net/browse/JDK-8042901
Webrev: http://cr.openjdk.java.net/~sjiang/JDK-8042901/00/

Some code within the module java.management is separated and moved to 
the new module jdk.management, the new module takes the implementation 
code for Oracle Corporation's platform extension to the implementation 
of the java.lang.management API and also the management interface for 
some other components for the platform.


Thanks,
Shanliang

RFR: 8073148 "The server has decided to close this client connection" repeated continuously

[shanliang]

Hi,

Please review this fix:

Bug: https://bugs.openjdk.java.net/browse/JDK-8073148
Webrev: http://cr.openjdk.java.net/~sjiang/JDK-8073148/00/

A JMX server was called to close, it terminated at first its 
notification forwarder server, then closed its communication connection 
(RMI), but the closing thread was blocked at the connection termination, 
so a client fetch request would continue arriving and the server would 
reply immediately with 0 notification.


The fix is to ask server to return null to a client fetching request in 
case of termination, this informs a ClientNotifForwarder of the server 
termination, so the client can safely stop fetching.


The fix was tested in the environment where the bug was reported.

Thanks,
Shanliang

Re: RFR: 8065213 Specify and implement PlatformMBeanProvider for looking for all platform MBeans

[shanliang]

Hi,

Hope this is last time :)
   http://cr.openjdk.java.net/~sjiang/JDK-8065213/02/

1)
Mandy Chung wrote:
You may want to consider using limited doPrivileged (that can be done 
in the second phase).

Done as in ManagementFactory:
878 // get all providers
879 List providers
880 = 
AccessController.doPrivileged((PrivilegedAction>) 
() -> {
881 List all = new 
ArrayList<>();
882 for (PlatformMBeanProvider provider : 
ServiceLoader.loadInstalled(PlatformMBeanProvider.class)) {

883 all.add(provider);
884 }
885 all.add(new DefaultPlatformMBeanProvider());
886 return all;
887 },
888 null,
889 new FilePermission("<>", "read"),
890 new 
RuntimePermission("sun.management.spi.PlatformMBeanProvider"));


2)
The modification to Flag is removed, we get another solution to know 
whether commercial feature is enabled.


3) some mis minors modifications.

Thanks,
Shanliang

  

Re: RFR: 8065213 Specify and implement PlatformMBeanProvider for looking for all platform MBeans

[shanliang]

Hi,

Thanks for all your comments, here is the new version:
   http://cr.openjdk.java.net/~sjiang/JDK-8065213/01/


Mandy Chung wrote:

ManagementFactory.java
   line 760: would be helpful to add some comments describing
   that the implementation of this method.  You can use
   MemoryManagerMXBean and GarageCollectorMXBean as the example.

Done.


   Comments for the addProxyNames

Changed the method name to getProxyNames and added the comments


   line 867: it would be more readable if you break this into
   two statements.
 List providers = 
AccessController.doPrivileged(...);

 providers.stream()
  .forEachOrdered(...);

Done

   line 875-886: worths formatting

   line 885 - instead of creating a HashMap and put entry in the 
forEach call.

   You could have do:
 .collect(toMap(PlatformComponent::getObjectNamePattern, 
Function.identity())

Yes, as:
   .collect(toMap(PlatformComponent::getObjectNamePattern, 
Function.identity(), (p1, p2) -> p1));


   You could also have the getMap() be called to initialize the 
componentMap
   static field (i.e. initialize it in the static initializer rather 
than lazily

   so the providers must be loaded anyway.

componentMap is now initialized statically.


   Can you add comments for the PlatformMBeanFinder methods?

Yes done


com/sun/management/internal/PlatformMBeanProviderImpl.java
   line 43: does this mxbeanList have to be created lazily?
   Would it be better to make it a final field and create it at the 
constructor?

Done as final

 line 58-59, 66-67, 111-112, 118-119: nit formatting

java/lang/management/DefaultPlatformMBeanProvider.java
   line 42: should this mxbeanList be a final field?

Done

   you can replace java.lang.management.MemoryManagerMXBean.class
   with MemoryManagerMXBean.class as it's in the same package.
   Same apply to other java.lang.management classes.

Done


com/sun/management/internal/PlatformMBeanProviderImpl.java
line 43: does this mxbeanList have to be created lazily?
Would it be better to make it a final field and create it at the
constructor?


These providers will need to be loaded and the mxbeanList will be 
looked at except for the cases that we are sure that the MXBean only 
comes from the default provider.  I see the cost of constructing 
mxbeanList involves loading several classes.  If performance is an 
issue, perhaps the fast-path would be in ManagementFactory to defer 
loading providers and may be better to follow up the performance side 
in the second phase (that I expect more changes to sun/management 
classes)


You may want to consider using limited doPrivileged (that can be done 
in the second phase).

OK, now they are final, I will add doPrivileged in next version.




Daniel Fuchs wrote:

ManagementFactory.java:

line 871: there's a stray debug trace that you probably
  forgot to remove:

871 System.out.println("\n\n= jsl adding: "+provider);

:)



lines 877-885:

 I believe it would improved readability if the content of the
 forEachOrdered statement was factored out in a private static
 method. Something like:

  .forEachOrdered(provider -> addToComponentMap(componentMap, provider))


 private static void addToComponentMap(
Map> cmpMap,
PlatformMBeanProvider provider) {
provider.getPlatformComponentList().stream()
.collect(toMap(p -> p.getObjectNamePattern(), p -> p,
   (p1, p2) -> {
   throw new InternalError(p1.getObjectNamePattern()
   + " has been used as key for " + p1
   + ", it cannot be reused for " + p2);
}))
   .values()
   .stream() // put all components into a map, "putIfAbsent"
   .forEach(p ->
   cmpMap.putIfAbsent(p.getObjectNamePattern(), p));
 }

You must know that code was changed :)


PlatformMBeanProviderImpl.java:

105  * 3 OperatingSystemMXBean

Not sure what '3' means here - I suggest to remove it.

OK

Thanks,
Shanliang

Re: RFR 8071641: java/lang/management/ThreadMXBean/SynchronizationStatistics.java intermittently failed with NPE

[shanliang]

Jaroslav Bachorik wrote:

On 29.1.2015 14:43, shanliang wrote:

Jaroslav Bachorik wrote:

Please, review the following test change.

Issue : https://bugs.openjdk.java.net/browse/JDK-8071641
Webrev: http://cr.openjdk.java.net/~jbachorik/8071641/webrev.00/

The test fails very intermittently with NPE. This seems to be caused
by a data race between Thread.getStatus() and
ThreadMXBean.getThreadInfo() - thread state is reported as BLOCKED but
the ThreadInfo still contains a stale information (lockname == null).

The solution would be re-retrieving the ThreadInfo until it reflects
the actual thread state.

-JB-

  77 private static void waitForThreadState(Thread t, Thread.State
state) throws InterruptedException {
  78 while (!t.isInterrupted() && t.getState() != state) {
  79 Thread.sleep(3);
  80 }
  81 }
Better to throw an exception if t.isInterrupted()?
Not sure when will happen t.isInterrupted(), but the method
waitForThreadState tests it, so it is supposed to be possible to get it,
when it is true then the testing method might return with a thread state
different to the waited one.


Yes, it might get to an undefined state. But given that the thread 
would get interrupted only when the harness is timing out the test 
this discrepancy would not actually matter - the test would fail with 
timeout anyway.

In this case it is useless to test t.isInterrupted() in waitForThreadState.

Shanliang


-JB-



Shanliang

Re: RFR 8071641: java/lang/management/ThreadMXBean/SynchronizationStatistics.java intermittently failed with NPE

[shanliang]

Jaroslav Bachorik wrote:

Please, review the following test change.

Issue : https://bugs.openjdk.java.net/browse/JDK-8071641
Webrev: http://cr.openjdk.java.net/~jbachorik/8071641/webrev.00/

The test fails very intermittently with NPE. This seems to be caused 
by a data race between Thread.getStatus() and 
ThreadMXBean.getThreadInfo() - thread state is reported as BLOCKED but 
the ThreadInfo still contains a stale information (lockname == null).


The solution would be re-retrieving the ThreadInfo until it reflects 
the actual thread state.


-JB-
 77 private static void waitForThreadState(Thread t, Thread.State 
state) throws InterruptedException {

 78 while (!t.isInterrupted() && t.getState() != state) {
 79 Thread.sleep(3);
 80 }
 81 }
Better to throw an exception if t.isInterrupted()?
Not sure when will happen t.isInterrupted(), but the method 
waitForThreadState tests it, so it is supposed to be possible to get it, 
when it is true then the testing method might return with a thread state 
different to the waited one.


Shanliang

RFR: 8065213 Specify and implement PlatformMBeanProvider for looking for all platform MBeans

[shanliang]

Hi,

Please review:

bug: https://bugs.openjdk.java.net/browse/JDK-8065213
webrev:   http://cr.openjdk.java.net/~sjiang/JDK-8065213/00/

The class PlatformMBeanProvider.java is added to allow different modules 
to inform ManagementFactory of their Plaform MBeans. In this version we 
use the ServiceLoader to load these Plaform MBeans, later we will 
replace the ServiceProvider by jigsaw mechanisms.


there are 2 providers in this version:
   DefaultPlatformMBeanProvider: for all MBeans in the 
java.lang.management.* (java.management module)
   com/sun/management/internal/PlatformMBeanProviderImpl: for the 
MBeans in com.sun.management.* com.sun.management.* (jdk.management module)
 
Thanks,

Shanliang

RFR: 8068774 CounterMonitorDeadlockTest.java timed out

[shanliang]

Hi

Please review this test bug fix

Bug: https://bugs.openjdk.java.net/browse/JDK-8068774
Webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068774/00/


The problem must be here:
98 monitorProxy.start();
99
100 final int initGetCount = observedProxy.getGetCount();

The test calls initGetCount after starting the monitor, but the test 
case 1 is:

 "Remove monitored MBean within monitored getAttribute"

that means if the monitor calls getAttribute before the test calls 
observedProxy.getGetCount(), then no more getAttribute will happen and 
the return of observedProxy.getGetCount() will not be changed any more. 
This is why the test is timeout.


I reproduced the bug by inserting at line 99:
 Thread.sleep(1000);

Thanks,
Shanliang

Re: RFR 8068591: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java should be quarantined

[shanliang]

Jaroslav Bachorik wrote:

On 8.1.2015 15:35, shanliang wrote:

Jaroslav Bachorik wrote:

Hi Shanliang,

On 8.1.2015 14:59, shanliang wrote:

Hi,

Please review this simple fix:

bug: https://bugs.openjdk.java.net/browse/JDK-8068591
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068591/00/


The entry is missing the label (eg. "generic-all")

Oops!
http://cr.openjdk.java.net/~sjiang/JDK-8068591/01/


Hm, shouldn't 
javax/management/remote/mandatory/connection/ReconnectTest.java be 
quarantined too?
Maybe, but better to have a new sub-bug of JDK-8042215 to quarantine 
ReconnectTest.java?


Shanliang


-JB-



Thanks,
Shanliang


-JB-



Thanks,
Shanliang

Re: RFR 8068591: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java should be quarantined

[shanliang]

Jaroslav Bachorik wrote:

Hi Shanliang,

On 8.1.2015 14:59, shanliang wrote:

Hi,

Please review this simple fix:

bug: https://bugs.openjdk.java.net/browse/JDK-8068591
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068591/00/


The entry is missing the label (eg. "generic-all")

Oops!
   http://cr.openjdk.java.net/~sjiang/JDK-8068591/01/

Thanks,
Shanliang


-JB-



Thanks,
Shanliang

RFR 8068591: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java should be quarantined

[shanliang]

Hi,

Please review this simple fix:

bug: https://bugs.openjdk.java.net/browse/JDK-8068591
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068591/00/

Thanks,
Shanliang

Re: RFR 6720349: NotificationBufferDeadlockTest.java throw exception: java.lang.Exception: TEST FAILED: Deadlock detected

[shanliang]

David Holmes wrote:

On 6/01/2015 9:03 PM, shanliang wrote:

David,

Here is the new version, I have added the comments as you suggested, and
I replaced the variable "sources" with a synchronized list.


Why did you do that ?? Vector is synchronized so it was fine for the job.

You are right! I have changed back to Vector:
   http://cr.openjdk.java.net/~sjiang/JDK-8068418/02/

I forgot Vetor is synchronized, so long time not use it.

Thanks,
Shanliang


David


http://cr.openjdk.java.net/~sjiang/JDK-8068418/01/

Thanks for the review.
Shanliang

David Holmes wrote:

Hi Shanliang,

On 6/01/2015 3:47 AM, shanliang wrote:

Hi,

Please review this fix:

bug: https://bugs.openjdk.java.net/browse/JDK-8068418
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068418/00/

This must be a timing issue in the test, the test called:
t.join(5000L); to wait a thread's dying,  I reproduced the bug by
insert at line 202:
try {
Thread.sleep(5100);
} catch (Exception ee) {}
to delay the t's dying.

The fix is to replace:
t.join(5000L);
by:
t.join();

and replace:
Object.wait(timeout);
by
CountDownLatch.countDown();


Okay - you could have just done wait() but the switch to
CountDownLatch is somewhat cleaner.


The test harness timeout will be used as the max waiting timeout.


So the test will now never report that it thinks it has hit a
deadlock, it will just timeout. But you added some extra printout so
okay I guess - but I suggest adding a comment at the top (where @run
etc are) saying that if test times out then deadlock is suspected.

Minor nit:

 System.out.println("DeadlockTest-addNotificationListener waiting the
XXX thread to die.

should say "waiting for the XXX thread ..."

Thanks,
David


Shanliang

Re: RFR 6720349: NotificationBufferDeadlockTest.java throw exception: java.lang.Exception: TEST FAILED: Deadlock detected

[shanliang]

David,

Here is the new version, I have added the comments as you suggested, and 
I replaced the variable "sources" with a synchronized list.

   http://cr.openjdk.java.net/~sjiang/JDK-8068418/01/

Thanks for the review.
Shanliang

David Holmes wrote:

Hi Shanliang,

On 6/01/2015 3:47 AM, shanliang wrote:

Hi,

Please review this fix:

bug: https://bugs.openjdk.java.net/browse/JDK-8068418
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068418/00/

This must be a timing issue in the test, the test called:
t.join(5000L); to wait a thread's dying,  I reproduced the bug by
insert at line 202:
try {
Thread.sleep(5100);
} catch (Exception ee) {}
to delay the t's dying.

The fix is to replace:
t.join(5000L);
by:
t.join();

and replace:
Object.wait(timeout);
by
CountDownLatch.countDown();


Okay - you could have just done wait() but the switch to 
CountDownLatch is somewhat cleaner.



The test harness timeout will be used as the max waiting timeout.


So the test will now never report that it thinks it has hit a 
deadlock, it will just timeout. But you added some extra printout so 
okay I guess - but I suggest adding a comment at the top (where @run 
etc are) saying that if test times out then deadlock is suspected.


Minor nit:

 System.out.println("DeadlockTest-addNotificationListener waiting the 
XXX thread to die.


should say "waiting for the XXX thread ..."

Thanks,
David


Shanliang

RFR 6720349: NotificationBufferDeadlockTest.java throw exception: java.lang.Exception: TEST FAILED: Deadlock detected

[shanliang]

Hi,

Please review this fix:

bug: https://bugs.openjdk.java.net/browse/JDK-8068418
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068418/00/

This must be a timing issue in the test, the test called:
   t.join(5000L);   
to wait a thread's dying,  I reproduced the bug by insert at line 202:

   try {
   Thread.sleep(5100);
   } catch (Exception ee) {}
to delay the t's dying.

The fix is to replace:
   t.join(5000L);
by:
   t.join();

and replace:
   Object.wait(timeout);
by
   CountDownLatch.countDown();

The test harness timeout will be used as the max waiting timeout.

Shanliang

RFR: 8066952 [TEST-BUG] javax/management/monitor/CounterMonitorTest.java hangs

[shanliang]

Hi,

As Daniel said: "I think that what happens here is that the 
StdObservedObject.getNbObjects() getter unblocks the main thread before 
returning the value to the monitor. This makes it possible to have a 
race condition where the next setNbObjects called by the main thread can 
then occur before the result of StdObservedObject.getNbObjects() is 
taken into account - and even before the 
StdObservedObject.getNbObjects() actually returns. So 
StdObservedObject.getNbObjects() may return a value different than the 
one which was observed. In our case, it would return 3 (the newer value) 
instead of 0 (the value that was observed). "


The suggested fix is to return "observedValue" instead of "count", in 
this way we check also that the monitor never starts next observation 
before the current one finishes.


bug: https://bugs.openjdk.java.net/browse/JDK-8066952
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8066952/00/

Thanks,
Shanliang

Re: RFR 8067241 DeadlockTest.java failed with negative timeout value

[shanliang]

Daniel Fuchs wrote:

Hi Shanliang,

These two statements are no longer needed and should be removed - as they
are misleading:

  64 if (!bb.gotLock) {
  65 throw new RuntimeException("Failed to get lock, impossible!");
  66 }


  81 if (!wb.done) {
  82 throw new RuntimeException("It is blocked!");
  83 }
  

Yes, can be removed.
  


which makes me wonder whether your changes are deeper than intended.
Now the test will either succeed, or fail in jtreg timeout. The condition
"It is blocked!"
can no longer be detected by the test. Is that your intention? If that's
your intention - I guess it is OK - but maybe it would deserve a comment
in the @summary of the test. Something like 'This bug attempts to 
reproduce

a deadlock situation and will block forever if the deadlock is present.'

Updated.

Here is the new version:
   http://cr.openjdk.java.net/~sjiang/JDK-8067241/01/

Thanks,
Shanliang



best regards,

-- daniel

On 12/12/14 8:33 AM, shanliang wrote:

Hi,

It is a test bug, it is not correct:
  while(!wb.done || timeToWait > 0) {

it should be:
  while(!wb.done && timeToWait > 0) {

|| should be changed to &&

Another issue is that the waiting time could be not enough (final 
long timeout = 2000).


The fix is to remove the waiting time specified in the test, the 
timeout of test harness will be used as the max waiting time.


bug: https://bugs.openjdk.java.net/browse/JDK-8067241
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8067241/00/

thanks,
Shanliang

RFR 8067241 DeadlockTest.java failed with negative timeout value

[shanliang]

Hi,

It is a test bug, it is not correct:
  while(!wb.done || timeToWait > 0) {

it should be:
  while(!wb.done && timeToWait > 0) {

|| should be changed to &&

Another issue is that the waiting time could be not enough (final long 
timeout = 2000).


The fix is to remove the waiting time specified in the test, the timeout 
of test harness will be used as the max waiting time.


bug: https://bugs.openjdk.java.net/browse/JDK-8067241
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8067241/00/

thanks,
Shanliang

Codereview: JDK-8065764 javax/management/monitor/CounterMonitorTest.java hangs

[shanliang]

Hi,
please review this test bug fix:

webrev:
http://cr.openjdk.java.net/~sjiang/JDK-8065764/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-8065764

The test tested the mode "difference", according to the Javadoc:
 If the counter difference mode is used, the value of the derived 
gauge is calculated as the difference between the observed counter 
values for two successive observations.


The test set the first value and then waited 2 times of 
granularityperiod at line 171, hoped that the monitor would get the 
first observation during this waiting time, but the test could fail 
because granularityperiod * 2 was not enough and the test did the second 
set before the monitor did the first observation.


It is easy to make the test timeout by commenting out the line 171.

The proposed solution is to get informed when the monitor did 
observation on calling:

   StdObservedObject.getNbObjects();

Thanks,
Shanliang

Re: Code review: 8046192 Eliminate SNMP dependencies to the internal APIs from open jdk modules

[shanliang]

Hi, Here is version 2:
   http://cr.openjdk.java.net/~sjiang/JDK-8046192/01/

The modification in ./jdk/src was missed in the first version.

The webreviews show only modification in the public part.

Thanks,
Shanliang

shanliang wrote:

Hi,

The fix is to remove unnecessary exports for jdk.snmp module.

bug: https://bugs.openjdk.java.net/browse/JDK-8046192
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8046192/00/

Thanks,
Shanliang

Code review: 8046192 Eliminate SNMP dependencies to the internal APIs from open jdk modules

[shanliang]

Hi,

The fix is to remove unnecessary exports for jdk.snmp module.

bug: https://bugs.openjdk.java.net/browse/JDK-8046192
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8046192/00/

Thanks,
Shanliang

Re: Codereview request: 8050115 javax/management/monitor/GaugeMonitorDeadlockTest.java fails intermittently

[shanliang]

David Holmes wrote:
Still not 100% sure about the deadlock detection logic here, but as 
long as it does no harm - ok.


Minor style nit:

  59 System.out.println("=== checkingTime = "+checkingTime+"ms");

spaces needed around the + operator.

OK, I have added spaces.

Thanks David for the review.
Shanliang


Thanks,
David

On 18/09/2014 1:09 AM, shanliang wrote:

Daniel Fuchs wrote:

On 9/17/14 4:43 PM, shanliang wrote:

Daniel,

We could not be sure that the test failed of timeout, that's why I 
tried

to add more checks.

The check for Step 1: all thread traces were printed out only if
deadlock was found, and the test failed immediately.
The check for Step 2:
1) all thread traces were printed out only if the tested thread 
was
blocked, but the test did not fail because we were not sure if 
deadlock

happened, but the info might be helpful;
2) otherwise only the trace of the tested thread was printed out.

In case that the test gets interrupted again by the test harness, hope
we can have some useful info from these 2 checks.

It must not be so heavy but still could impact the test, your 
suggestion
to use test.timeout.factor is a good idea, I added the code to 
calculate

the checking time based on it:
http://cr.openjdk.java.net/~sjiang/JDK-8050115/03/


I Shanliang,

This looks much better, thanks.
May I suggested taking the current time again at lines 125
and 179:

   checkedTime = System.currentTimeMillis();

It would allow to discount the time spent in checking.
Here is the new version with your suggestion to re-calculate 
checkedTime.


http://cr.openjdk.java.net/~sjiang/JDK-8050115/04/

Thanks a lot for your time!
Shanliang


best regards,

-- daniel



Thanks,
Shanliang

Daniel Fuchs wrote:

Hi Shanliang,

On 9/17/14 2:19 PM, shanliang wrote:

Daniel,

The test does 2 steps of verifications, the new check is useful for
the
first step, and the trace in the bug showed that the test failed on
the
first step.

Yes the check might not work for the second step, I added the new 
code
for the second step to check the tested thread state and hope to 
have

useful info if the test failed on the second step.

Here is the new version:
http://cr.openjdk.java.net/~sjiang/JDK-8050115/02/

Thanks,
Shanliang


If I understand the issue correctly - the test fails in timeout
mostly on very slow machines/configurations (fastdebug with some
combinations of options).

I worry that printing a thread dump every seconds (1000ms) is going
to make things worse: the test will spend its time printing thread
dumps instead of doing what it is supposed to do - and will have
even less CPU cycles to execute its 'real' code.

I would have advised printing the thread dumps only at the end,
when it is detected that there might be a deadlock - except that
now we can't do that since the timeout is managed completely
by the harness (so we don't get the upper hand at the end in
case of timeout).

I think depending on the harness to set the appropriate timeout
rather than depending on an arbitrary timeout set in the test itself
is the right thing to do. It's been a pattern in many tests that
failed in timeout intermittently on some slow machines/configuration.

In any case - 1s seems really too frequent.
I suppose you could inspect the system properties set by the harness
(timeout + timeout factor) to devise an acceptable frequency for
your checks - if you really want to print this info.

From the log I see that the timeout factor passed to the harness
for the slow configuration that failed is
-Dtest.timeout.factor=8.0
There's no explicit timeout given - and jtreg -onlineHelp reveals
that in this case the default timeout is two minutes.

This means that the harness has allocated 2*8=16mins for the test to
execute.
I don't think you want to take the risk of printing a thread dump
every seconds during 16 minutes ;-)

Of course I'm over simplifying here. Before your changes - the test
was deciding after 46.893 seconds that there must be a deadlock.
47s is obviously way too short for a possibly slow machine running
the test in fastdebug mode.

Something like the following might be more reasonable:

// default timeout factor is 1.0
double factor =
Double.parseDouble(
   System.getProperty("test.timeout.factor", "1.0"));
// default timeout is 2mins = 120s.
double timeout = Double.parseDouble(
   System.getProperty("test.timeout", "120"));

// total time is timeout * timeout factor * 1000 ms
long total = (long) factor * timeout * 1000;

// Don't print thread dumps too often.
// every 5s for a total timeout of 120s seems reasonable.
// 120s/5s = 24; we will lengthen the delay if the total
// timeout is greater than 120s, so we're taking the max between
// 5s and total/24
long delayBetweenThreadDumps = Math.max(5000, total/24);

Of course 5s and total/24 are just arbitrary...
But 24 full threa

Re: Codereview request: 8050115 javax/management/monitor/GaugeMonitorDeadlockTest.java fails intermittently

[shanliang]

Daniel Fuchs wrote:

On 9/17/14 4:43 PM, shanliang wrote:

Daniel,

We could not be sure that the test failed of timeout, that's why I tried
to add more checks.

The check for Step 1: all thread traces were printed out only if
deadlock was found, and the test failed immediately.
The check for Step 2:
1) all thread traces were printed out only if the tested thread was
blocked, but the test did not fail because we were not sure if deadlock
happened, but the info might be helpful;
2) otherwise only the trace of the tested thread was printed out.

In case that the test gets interrupted again by the test harness, hope
we can have some useful info from these 2 checks.

It must not be so heavy but still could impact the test, your suggestion
to use test.timeout.factor is a good idea, I added the code to calculate
the checking time based on it:
http://cr.openjdk.java.net/~sjiang/JDK-8050115/03/


I Shanliang,

This looks much better, thanks.
May I suggested taking the current time again at lines 125
and 179:

   checkedTime = System.currentTimeMillis();

It would allow to discount the time spent in checking.

Here is the new version with your suggestion to re-calculate checkedTime.

   http://cr.openjdk.java.net/~sjiang/JDK-8050115/04/

Thanks a lot for your time!
Shanliang


best regards,

-- daniel



Thanks,
Shanliang

Daniel Fuchs wrote:

Hi Shanliang,

On 9/17/14 2:19 PM, shanliang wrote:

Daniel,

The test does 2 steps of verifications, the new check is useful for 
the
first step, and the trace in the bug showed that the test failed on 
the

first step.

Yes the check might not work for the second step, I added the new code
for the second step to check the tested thread state and hope to have
useful info if the test failed on the second step.

Here is the new version:
http://cr.openjdk.java.net/~sjiang/JDK-8050115/02/

Thanks,
Shanliang


If I understand the issue correctly - the test fails in timeout
mostly on very slow machines/configurations (fastdebug with some
combinations of options).

I worry that printing a thread dump every seconds (1000ms) is going
to make things worse: the test will spend its time printing thread
dumps instead of doing what it is supposed to do - and will have
even less CPU cycles to execute its 'real' code.

I would have advised printing the thread dumps only at the end,
when it is detected that there might be a deadlock - except that
now we can't do that since the timeout is managed completely
by the harness (so we don't get the upper hand at the end in
case of timeout).

I think depending on the harness to set the appropriate timeout
rather than depending on an arbitrary timeout set in the test itself
is the right thing to do. It's been a pattern in many tests that
failed in timeout intermittently on some slow machines/configuration.

In any case - 1s seems really too frequent.
I suppose you could inspect the system properties set by the harness
(timeout + timeout factor) to devise an acceptable frequency for
your checks - if you really want to print this info.

From the log I see that the timeout factor passed to the harness
for the slow configuration that failed is
-Dtest.timeout.factor=8.0
There's no explicit timeout given - and jtreg -onlineHelp reveals
that in this case the default timeout is two minutes.

This means that the harness has allocated 2*8=16mins for the test to
execute.
I don't think you want to take the risk of printing a thread dump
every seconds during 16 minutes ;-)

Of course I'm over simplifying here. Before your changes - the test
was deciding after 46.893 seconds that there must be a deadlock.
47s is obviously way too short for a possibly slow machine running
the test in fastdebug mode.

Something like the following might be more reasonable:

// default timeout factor is 1.0
double factor =
Double.parseDouble(
   System.getProperty("test.timeout.factor", "1.0"));
// default timeout is 2mins = 120s.
double timeout = Double.parseDouble(
   System.getProperty("test.timeout", "120"));

// total time is timeout * timeout factor * 1000 ms
long total = (long) factor * timeout * 1000;

// Don't print thread dumps too often.
// every 5s for a total timeout of 120s seems reasonable.
// 120s/5s = 24; we will lengthen the delay if the total
// timeout is greater than 120s, so we're taking the max between
// 5s and total/24
long delayBetweenThreadDumps = Math.max(5000, total/24);

Of course 5s and total/24 are just arbitrary...
But 24 full thread dumps in a log for a single test is enough data
to analyze I think ;-)

best regards,

-- daniel




Daniel Fuchs wrote:

On 9/17/14 10:55 AM, shanliang wrote:

David Holmes wrote:

On 17/09/2014 7:01 AM, shanliang wrote:

David Holmes wrote:

Hi Shanliang,

On 16/09/2014 7:12 PM, shanliang wrote:

Hi,

Please review the following fix:


I don't see any functional change. You seem to have rep

Re: Codereview request: 8050115 javax/management/monitor/GaugeMonitorDeadlockTest.java fails intermittently

[shanliang]

Daniel,

We could not be sure that the test failed of timeout, that's why I tried 
to add more checks.


The check for Step 1: all thread traces were printed out only if 
deadlock was found, and the test failed immediately.

The check for Step 2:
   1) all thread traces were printed out only if the tested thread was 
blocked, but the test did not fail because we were not sure if deadlock 
happened, but the info might be helpful;

   2) otherwise only the trace of the tested thread was printed out.

In case that the test gets interrupted again by the test harness, hope 
we can have some useful info from these 2 checks.


It must not be so heavy but still could impact the test, your suggestion 
to use test.timeout.factor is a good idea, I added the code to calculate 
the checking time based on it:

   http://cr.openjdk.java.net/~sjiang/JDK-8050115/03/

Thanks,
Shanliang

Daniel Fuchs wrote:

Hi Shanliang,

On 9/17/14 2:19 PM, shanliang wrote:

Daniel,

The test does 2 steps of verifications, the new check is useful for the
first step, and the trace in the bug showed that the test failed on the
first step.

Yes the check might not work for the second step, I added the new code
for the second step to check the tested thread state and hope to have
useful info if the test failed on the second step.

Here is the new version:
http://cr.openjdk.java.net/~sjiang/JDK-8050115/02/

Thanks,
Shanliang


If I understand the issue correctly - the test fails in timeout
mostly on very slow machines/configurations (fastdebug with some
combinations of options).

I worry that printing a thread dump every seconds (1000ms) is going
to make things worse: the test will spend its time printing thread
dumps instead of doing what it is supposed to do - and will have
even less CPU cycles to execute its 'real' code.

I would have advised printing the thread dumps only at the end,
when it is detected that there might be a deadlock - except that
now we can't do that since the timeout is managed completely
by the harness (so we don't get the upper hand at the end in
case of timeout).

I think depending on the harness to set the appropriate timeout
rather than depending on an arbitrary timeout set in the test itself
is the right thing to do. It's been a pattern in many tests that
failed in timeout intermittently on some slow machines/configuration.

In any case - 1s seems really too frequent.
I suppose you could inspect the system properties set by the harness
(timeout + timeout factor) to devise an acceptable frequency for
your checks - if you really want to print this info.

From the log I see that the timeout factor passed to the harness
for the slow configuration that failed is
-Dtest.timeout.factor=8.0
There's no explicit timeout given - and jtreg -onlineHelp reveals
that in this case the default timeout is two minutes.

This means that the harness has allocated 2*8=16mins for the test to
execute.
I don't think you want to take the risk of printing a thread dump
every seconds during 16 minutes ;-)

Of course I'm over simplifying here. Before your changes - the test
was deciding after 46.893 seconds that there must be a deadlock.
47s is obviously way too short for a possibly slow machine running
the test in fastdebug mode.

Something like the following might be more reasonable:

// default timeout factor is 1.0
double factor =
Double.parseDouble(
   System.getProperty("test.timeout.factor", "1.0"));
// default timeout is 2mins = 120s.
double timeout = Double.parseDouble(
   System.getProperty("test.timeout", "120"));

// total time is timeout * timeout factor * 1000 ms
long total = (long) factor * timeout * 1000;

// Don't print thread dumps too often.
// every 5s for a total timeout of 120s seems reasonable.
// 120s/5s = 24; we will lengthen the delay if the total
// timeout is greater than 120s, so we're taking the max between
// 5s and total/24
long delayBetweenThreadDumps = Math.max(5000, total/24);

Of course 5s and total/24 are just arbitrary...
But 24 full thread dumps in a log for a single test is enough data
to analyze I think ;-)

best regards,

-- daniel




Daniel Fuchs wrote:

On 9/17/14 10:55 AM, shanliang wrote:

David Holmes wrote:

On 17/09/2014 7:01 AM, shanliang wrote:

David Holmes wrote:

Hi Shanliang,

On 16/09/2014 7:12 PM, shanliang wrote:

Hi,

Please review the following fix:


I don't see any functional change. You seem to have replaced a
built-in timeout with the externally applied test harness timeout.

Yes no functional change here, we thought that the test needed more
time
to wait a change if a testing VM or machine was really slow, the 
test

harness timeout was the maximum time we could give the test.


Do we have confidence that the harness timeout is sufficient to 
handle

the intermittent failures?

Really a good question :)

Here is new version:
http://cr.openjdk.java.net/~sj

Re: Codereview request: 8050115 javax/management/monitor/GaugeMonitorDeadlockTest.java fails intermittently

[shanliang]

Daniel,

The test does 2 steps of verifications, the new check is useful for the 
first step, and the trace in the bug showed that the test failed on the 
first step.


Yes the check might not work for the second step, I added the new code 
for the second step to check the tested thread state and hope to have 
useful info if the test failed on the second step.


Here is the new version:
   http://cr.openjdk.java.net/~sjiang/JDK-8050115/02/

Thanks,
Shanliang


Daniel Fuchs wrote:

On 9/17/14 10:55 AM, shanliang wrote:

David Holmes wrote:

On 17/09/2014 7:01 AM, shanliang wrote:

David Holmes wrote:

Hi Shanliang,

On 16/09/2014 7:12 PM, shanliang wrote:

Hi,

Please review the following fix:


I don't see any functional change. You seem to have replaced a
built-in timeout with the externally applied test harness timeout.
Yes no functional change here, we thought that the test needed more 
time

to wait a change if a testing VM or machine was really slow, the test
harness timeout was the maximum time we could give the test.


Do we have confidence that the harness timeout is sufficient to handle
the intermittent failures?

Really a good question :)

Here is new version:
http://cr.openjdk.java.net/~sjiang/JDK-8050115/01/

I added a deadlocked check in every 1 second, hope to get more info in
case of failure.


The following comment seems to imply that this check is not
very useful:

 112 // This won't show up as a deadlock in CTRL-\ or in
 113 // ThreadMXBean.findDeadlockedThreads(), because they 
don't
 114 // see that thread A is waiting for thread B 
(B.join()), and

 115 // thread B is waiting for a lock held by thread A

best regards,

-- daniel



I changed also the sleep time to 100ms, 10ms seems too short as Daniel
pointed out.

Thanks,
Shanliang


Thanks,
David




Style nit: add a space after 'while' -> while (cond) {

OK, I will do it before pushing.

Thanks,
Shanliang


David
-


bug: https://bugs.openjdk.java.net/browse/JDK-8050115
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8050115/00/

Thanks,
Shanliang

Re: Codereview request: 8050115 javax/management/monitor/GaugeMonitorDeadlockTest.java fails intermittently

[shanliang]

David Holmes wrote:

On 17/09/2014 7:01 AM, shanliang wrote:

David Holmes wrote:

Hi Shanliang,

On 16/09/2014 7:12 PM, shanliang wrote:

Hi,

Please review the following fix:


I don't see any functional change. You seem to have replaced a
built-in timeout with the externally applied test harness timeout.

Yes no functional change here, we thought that the test needed more time
to wait a change if a testing VM or machine was really slow, the test
harness timeout was the maximum time we could give the test.


Do we have confidence that the harness timeout is sufficient to handle 
the intermittent failures?

Really a good question :)

Here is new version:
   http://cr.openjdk.java.net/~sjiang/JDK-8050115/01/

I added a deadlocked check in every 1 second, hope to get more info in 
case of failure.


I changed also the sleep time to 100ms, 10ms seems too short as Daniel 
pointed out.


Thanks,
Shanliang


Thanks,
David




Style nit: add a space after 'while' -> while (cond) {

OK, I will do it before pushing.

Thanks,
Shanliang


David
-


bug: https://bugs.openjdk.java.net/browse/JDK-8050115
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8050115/00/

Thanks,
Shanliang

Re: Codereview request: 8050115 javax/management/monitor/GaugeMonitorDeadlockTest.java fails intermittently

[shanliang]

David Holmes wrote:

Hi Shanliang,

On 16/09/2014 7:12 PM, shanliang wrote:

Hi,

Please review the following fix:


I don't see any functional change. You seem to have replaced a 
built-in timeout with the externally applied test harness timeout.
Yes no functional change here, we thought that the test needed more time 
to wait a change if a testing VM or machine was really slow, the test 
harness timeout was the maximum time we could give the test. 


Style nit: add a space after 'while' -> while (cond) {

OK, I will do it before pushing.

Thanks,
Shanliang


David
-


bug: https://bugs.openjdk.java.net/browse/JDK-8050115
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8050115/00/

Thanks,
Shanliang

Re: jmx-dev Codereview request: 8050115 javax/management/monitor/GaugeMonitorDeadlockTest.java fails intermittently

[shanliang]

Daniel Fuchs wrote:

Hi Shanliang,

line 116 - you could use a CountDownLatch instead of an
AtomicInteger. It would avoid having to use the busy loop at
lines 134-136.
Yes CountDownLatch is really a good idea, I tried to modify the code as 
less as possible, I prefer to keep the old code this time, another 
reason is that we still need to do same thing at line 103.


I also wonder whether you could increase the sleep timeout
at line 107 - to make that loop a bit less buzy.
Unless that would alter the test too much and make the
deadlock less probable?

Line 95: monitorProxy.setGranularityPeriod(10L); // 10 ms
So waiting 10ms seems reasonable.


Otherwise the changes look reasonable.

Have you tried reproducing the failure (before fixing) on your
own machine using the same config than what was reported to
fail? (fastdebug build with all the -server + -XX etc options?)

I reproduced the bug only by reducing the waiting time.

Thanks Daniel!
Shanliang


best regards,

-- daniel


On 9/16/14 11:12 AM, shanliang wrote:

Hi,

Please review the following fix:

bug: https://bugs.openjdk.java.net/browse/JDK-8050115
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8050115/00/

Thanks,
Shanliang

Codereview request: 8050115 javax/management/monitor/GaugeMonitorDeadlockTest.java fails intermittently

[shanliang]

Hi,

Please review the following fix:

bug: https://bugs.openjdk.java.net/browse/JDK-8050115
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8050115/00/

Thanks,
Shanliang

Re: Review request: 8042205: javax/management/monitor/*: some test didn't get all the notifications

[shanliang]

Daniel Fuchs wrote:

Looks good Shanliang.

The synchronization is a bit strange, with the flag being
volatile and sometime modified within synchronized blocks and
sometime being modified outside of any s-block, but I believe
it is working (AFAIU the synchronized is mostly needed because
you call notifyAll() and wait() and the fact that the flag is
also modified within the block is just coincidence ;-) ).
I'm OK with this.
Indeed it is not necessary to modify the flag within the 
synchronization, but is harmless and "coincidence".


Here is the new version with the modification for test/ProblemList.txt, 
simply removing the tests modified.


http://cr.openjdk.java.net/~sjiang/JDK-8042205/01/

Thanks,
Daniel


-- daniel

On 9/15/14 3:05 PM, shanliang wrote:

Hi,

Please review the following fix, I changed the way to check received
notifications.

Bug: https://bugs.openjdk.java.net/browse/JDK-8042205
Webrec: http://cr.openjdk.java.net/~sjiang/JDK-8042205/00/
<http://cr.openjdk.java.net/%7Esjiang/JDK-8042205/00/>

Thanks, shanliang

Review request: 8042205: javax/management/monitor/*: some test didn't get all the notifications

[shanliang]

Hi,

Please review the following fix, I changed the way to check received 
notifications.


Bug: https://bugs.openjdk.java.net/browse/JDK-8042205
Webrec: http://cr.openjdk.java.net/~sjiang/JDK-8042205/00/ 
<http://cr.openjdk.java.net/%7Esjiang/JDK-8042205/00/>


Thanks, shanliang

Review request: JDK-8057951: javax/management/monitor/... should be quarantined

[shanliang]

Hi,

Please review this simple modification to ProblemList.txt:

bug: https://bugs.openjdk.java.net/browse/JDK-8057951

diff -r 1ebd76247e3e test/ProblemList.txt
--- a/test/ProblemList.txtFri Sep 12 12:19:27 2014 +0200
+++ b/test/ProblemList.txtFri Sep 12 17:37:38 2014 +0200
@@ -142,6 +142,13 @@
# 8050115
javax/management/monitor/GaugeMonitorDeadlockTest.java  generic-all

+# 8042205  8057951  generic-all
+javax/management/monitor/CounterMonitorTest.java
+javax/management/monitor/NonComparableAttributeValueTest.java
+javax/management/monitor/ReflectionExceptionTest.java
+javax/management/monitor/RuntimeExceptionTest.java
+javax/management/monitor/AttributeArbitraryDataTypeTest.java
+


Thanks,
Shanliang

Re: jmx-dev Review request: 8049303: Transient network problems cause JMX thread to fail silenty

[shanliang]

Daniel Fuchs wrote:

On 9/10/14 9:45 PM, shanliang wrote:

Oh, not one retry attempt fetching the next batch of notifications, but
the *SAME* batch of notifications.

http://cr.openjdk.java.net/~sjiang/JDK-8049303/02/
<http://cr.openjdk.java.net/%7Esjiang/JDK-8049303/02/>

Shanliang




This looks good Shanliang!

Make sure to rerun all the JCK/JDK tests before pushing.

Yes already ran, but will do once again to make sure.

This was really a tricky problem!

Yes, it is!

Thanks for your help!
Shanliang


best regards,

-- daniel

Re: jmx-dev Review request: 8049303: Transient network problems cause JMX thread to fail silenty

[shanliang]

shanliang wrote:

Daniel Fuchs wrote:

Hi,

Thanks for the detailed explanations.

The fact that the server doesn't store any client state
and can arbitrarily close the connection, leaving it to
the client to reestablish the connection, makes all this
code quite tricky and hard to follow.
Yes it is complicated, we allowed a server to close a client after a 
specific timeout because in some case a client was dead but the server 
needed long long time to be informed, that could make memory problem.


I believe what you propose - making sure that the
notification thread doesn't stop without closing the
connection, is the right thing to do.

I wonder however if the code that closes the connection
should better be moved to ClientNotifForwarder::fetchNotifs?

ClientNotifForwarder::fetchNotifs has the following statement:

601 if (!shouldStop()) {
602 logger.error("NotifFetcher-run",
603  "Failed to fetch notification, " +
604  "stopping thread. Error is: " + ioe, ioe);
605 logger.debug("NotifFetcher-run",ioe);
607 }

Then it proceeds to return null, which causes the thread to die.
ClientNotifForwarder is an abstract super class and does not know how 
to close a connection, this class is extended by 
RMIConnector.RMINotifClient and JMXMP, if we modify the class to have 
connection reference, that might make problem for JMXMP.




It looks as if that's the place where the connection should ideally
be closed if it is not already closed, because it would ensure
that the thread never dies silently.

Otherwise I'd suggest improving the comment below:

1369 // JDK-8049303
1370 // possible again transient or a
1371 // non-deserialization exception, 
not know how

1372 // to treat, close the connection

May I suggest something like:

// JDK-8049303
// We received an IOException - but the communicatorAdmin
// did not close the connection - possibly because the
// the original exception was raised by a transient network
// problem?
// We already know that this exception is not due to a deserialization
// issue as we already took care of that before involving the
// communicatorAdmin. Moreover - we already made one retry attempt
// at fetching the next batch of notifications - and the
// problem persisted.
// Since trying again doesn't seem to solve the issue, we will now
// close the connection. Doing otherwise might cause the
// NotifFetcher thread to die silently.

Yes more clear, here is the new webrev:

http://cr.openjdk.java.net/~sjiang/JDK-8049303/01/
Oh, not one retry attempt fetching the next batch of notifications, but 
the *SAME* batch of notifications.


http://cr.openjdk.java.net/~sjiang/JDK-8049303/02/ 
<http://cr.openjdk.java.net/%7Esjiang/JDK-8049303/02/>


Shanliang


Thanks Daniel!
Shanliang


best regards,

-- daniel

On 9/10/14 5:42 PM, shanliang wrote:

Hi,

The issue could happen like this:
1) RMIConnector.RMINotifClient.fetchNotifs got an IOException
2) communicatorAdmin.gotIOException(ioe) was called to check the
connection, it did not close the connection because the connection was
now OK.
3) RMIConnector.RMINotifClient.fetchNotifs analyzed the original
exception and found it was not a dersialization exception, it re-threw
the original IOException
4) the caller ClientNotifForwarder did not know how to treat this
exception, decided to end silently.

The fix is to modify RMIConnector.RMINotifClient.fetchNotifs:

if the fetchNotifs request gets an IOException, we examine the chain of
exceptions to determine whether this is a deserialization issue. If 
so -

we propagate the appropriate exception to the caller, who will then
proceed with fetching notifications one by one, otherwise we call
communicatorAdmin.gotIOException(ioe), there are 2 kinds of response:
1) the call returns OK, means the connection is re-established, we
re-call the fetchNotifs;
2) the call throws IOException, we check the connection status:
2-1) "terminated", that means the connection is closed, we
re-throw the original IOException, the caller will end silently.
2-2) not "terminated", we add a flag "retried" for this
situation, if the flag is false, we set the flag to true and re-do the
fetchNotifs request, this is useful for a transient network problem,
otherwise we close the connection and re-throw the original 
IOException,

it is here we fix the bug.

We do not modify communicatorAdmin.gotIOException(ioe), it is called 
too

by all other remote requests.

It is not easy to have a test reproducing the bug.

Bug: https://bugs.openjdk.java.net/browse/JDK-8049303
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8049303/00/
<http://cr.openjdk.java.net/%7Esjiang/JDK-8049303/00/>

Thanks,
Shanliang

Re: Review request: 8049303: Transient network problems cause JMX thread to fail silenty

[shanliang]

Daniel Fuchs wrote:

Hi,

Thanks for the detailed explanations.

The fact that the server doesn't store any client state
and can arbitrarily close the connection, leaving it to
the client to reestablish the connection, makes all this
code quite tricky and hard to follow.
Yes it is complicated, we allowed a server to close a client after a 
specific timeout because in some case a client was dead but the server 
needed long long time to be informed, that could make memory problem.


I believe what you propose - making sure that the
notification thread doesn't stop without closing the
connection, is the right thing to do.

I wonder however if the code that closes the connection
should better be moved to ClientNotifForwarder::fetchNotifs?

ClientNotifForwarder::fetchNotifs has the following statement:

601 if (!shouldStop()) {
602 logger.error("NotifFetcher-run",
603  "Failed to fetch notification, " +
604  "stopping thread. Error is: " + ioe, ioe);
605 logger.debug("NotifFetcher-run",ioe);
607 }

Then it proceeds to return null, which causes the thread to die.
ClientNotifForwarder is an abstract super class and does not know how to 
close a connection, this class is extended by 
RMIConnector.RMINotifClient and JMXMP, if we modify the class to have 
connection reference, that might make problem for JMXMP.




It looks as if that's the place where the connection should ideally
be closed if it is not already closed, because it would ensure
that the thread never dies silently.

Otherwise I'd suggest improving the comment below:

1369 // JDK-8049303
1370 // possible again transient or a
1371 // non-deserialization exception, not 
know how

1372 // to treat, close the connection

May I suggest something like:

// JDK-8049303
// We received an IOException - but the communicatorAdmin
// did not close the connection - possibly because the
// the original exception was raised by a transient network
// problem?
// We already know that this exception is not due to a deserialization
// issue as we already took care of that before involving the
// communicatorAdmin. Moreover - we already made one retry attempt
// at fetching the next batch of notifications - and the
// problem persisted.
// Since trying again doesn't seem to solve the issue, we will now
// close the connection. Doing otherwise might cause the
// NotifFetcher thread to die silently.

Yes more clear, here is the new webrev:

http://cr.openjdk.java.net/~sjiang/JDK-8049303/01/

Thanks Daniel!
Shanliang


best regards,

-- daniel

On 9/10/14 5:42 PM, shanliang wrote:

Hi,

The issue could happen like this:
1) RMIConnector.RMINotifClient.fetchNotifs got an IOException
2) communicatorAdmin.gotIOException(ioe) was called to check the
connection, it did not close the connection because the connection was
now OK.
3) RMIConnector.RMINotifClient.fetchNotifs analyzed the original
exception and found it was not a dersialization exception, it re-threw
the original IOException
4) the caller ClientNotifForwarder did not know how to treat this
exception, decided to end silently.

The fix is to modify RMIConnector.RMINotifClient.fetchNotifs:

if the fetchNotifs request gets an IOException, we examine the chain of
exceptions to determine whether this is a deserialization issue. If so -
we propagate the appropriate exception to the caller, who will then
proceed with fetching notifications one by one, otherwise we call
communicatorAdmin.gotIOException(ioe), there are 2 kinds of response:
1) the call returns OK, means the connection is re-established, we
re-call the fetchNotifs;
2) the call throws IOException, we check the connection status:
2-1) "terminated", that means the connection is closed, we
re-throw the original IOException, the caller will end silently.
2-2) not "terminated", we add a flag "retried" for this
situation, if the flag is false, we set the flag to true and re-do the
fetchNotifs request, this is useful for a transient network problem,
otherwise we close the connection and re-throw the original IOException,
it is here we fix the bug.

We do not modify communicatorAdmin.gotIOException(ioe), it is called too
by all other remote requests.

It is not easy to have a test reproducing the bug.

Bug: https://bugs.openjdk.java.net/browse/JDK-8049303
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8049303/00/
<http://cr.openjdk.java.net/%7Esjiang/JDK-8049303/00/>

Thanks,
Shanliang

Review request: 8049303: Transient network problems cause JMX thread to fail silenty

[shanliang]

Hi,

The issue could happen like this:
1) RMIConnector.RMINotifClient.fetchNotifs got an IOException
2) communicatorAdmin.gotIOException(ioe) was called to check the 
connection, it did not close the connection because the connection was 
now OK.
3) RMIConnector.RMINotifClient.fetchNotifs analyzed the original 
exception and found it was not a dersialization exception, it re-threw 
the original IOException
4) the caller ClientNotifForwarder did not know how to treat this 
exception, decided to end silently.


The fix is to modify RMIConnector.RMINotifClient.fetchNotifs:

if the fetchNotifs request gets an IOException, we examine the chain of 
exceptions to determine whether this is a deserialization issue. If so - 
we propagate the appropriate exception to the caller, who will then 
proceed with fetching notifications one by one, otherwise we call 
communicatorAdmin.gotIOException(ioe), there are 2 kinds of response:
   1) the call returns OK, means the connection is re-established, we 
re-call the fetchNotifs;

   2) the call throws IOException, we check the connection status:
   2-1) "terminated", that means the connection is closed, we 
re-throw the original IOException, the caller will end silently.
   2-2) not "terminated", we add a flag "retried" for this 
situation, if the flag is false, we set the flag to true and re-do the 
fetchNotifs request, this is useful for a transient network problem, 
otherwise we close the connection and re-throw the original IOException, 
it is here we fix the bug.


We do not modify communicatorAdmin.gotIOException(ioe), it is called too 
by all other remote requests.


It is not easy to have a test reproducing the bug.

Bug: https://bugs.openjdk.java.net/browse/JDK-8049303
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8049303/00/ 
<http://cr.openjdk.java.net/%7Esjiang/JDK-8049303/00/>


Thanks,
Shanliang

Re: jmx-dev Review request: 8049303: Transient network problems cause JMX thread to fail silenty

[shanliang]

Jaroslav,

Your fix was to close a connection if the IOException was not related to 
a serialization problem, without testing whether the connection was back.


This might modify the current RMIConnector behaviors, because the method
   RMIClientCommunicatorAdmin.gotIOException
was called not only by a notification fetching thread, it is called by 
all remote requests of a RMI client, look at:

   RMIConnector.createMBean
   try {
   return connection.createMBean(className,
   name,
   loaderName,
   delegationSubject);

   } catch (IOException ioe) {
   communicatorAdmin.gotIOException(ioe);

   return connection.createMBean(className,
   name,
   loaderName,
   delegationSubject);

   } finally {
   popDefaultClassLoader(old);
   }

with the suggested fix, no more second call of connection.createMBean 
(Yes, we need more tests to cover these cases).


So a fix is better added in RMIConnector.RMINotifClient.fetchNotifs.

Thanks,
Shanliang

Jaroslav Bachorik wrote:

On 08/29/2014 11:25 AM, Daniel Fuchs wrote:

Hi Jaroslav,

I am not sure to understand how this solves the problem.
The old code first checked the connection, and if that failed,
sent the FAILED notification, closed the connector, and rethrew
the exception.


This problem seems to have something to do with the way RMI works - 
the customer had problems with one set of ties/stubs while the other 
set of ties/stubs worked just fine. Seems like in cases of transient 
network failures the connection check was not reliable.




The new code directly throws the exception without
checking the connection, and therefore without closing
the connection and sending the FAILED notification.


It only does so for the cases where the connection itself is not the 
culprit - error while executing the method on the server, marshalling 
problems etc.




So is the fix a change of behavior by which the RMIConnector
will - in some cases - not try to autoclose the connection but
instead simply wait for the caller to explicitely call close()?


Not really - the change is in relying on the RMI providing the 
information whether the connection is still usable or not. The code 
didn't autoclose the connection when 
"connection.getDefaultDomain(null)" didn't throw IOException either.




I'd be interested to hear what Shanliang has to say...


Yep. The code does a lot of things at once and without any spec for 
handling failures and recovery we can only rely on the tests.


-JB-



best regards,

-- daniel


On 8/28/14 5:57 PM, Jaroslav Bachorik wrote:

I have taken over this issue from Poonam since she will be unavailable
for the next month or so.

Could I have reviews for this change:

Bug: https://bugs.openjdk.java.net/browse/JDK-8049303
Webrev: http://cr.openjdk.java.net/~jbachorik/8049303/webrev.00

Problem and fix:
By default the JMX client side notification fetch timeout
(jmx.remote.x.notification.fetch.timeout) is 1 minute and the default
server connection timeout (jmx.remote.x.server.connection.timeout) is 2
minutes.

If the client side connector thread makes a notification fetch request
to the server, but a transient network problem prevents the server
response from reaching the client, the client side connector will wait
for a response until the timeout period (1 minute) has expired before
throwing an IOException.

The client side RMIConnector implementation handles the IOException, by
re-checking the connection status to understand whether or not it is
broken. If the connection is not available at that moment, the 
connector

fails by re-throwing the initial IOException. The problem is that this
re-check of the connection passes because the server side of the
connection doesn't time out until 2 minutes has passed (by default), so
the NotifFetcher thread
dies without posting a failed notification, and the client application
does not get a chance to recover.

The fix is to forward the non connection-related exceptions on the JMX
client side instead of checking the connection status. The
connection-related exceptions will cause closing the session as an
unsuccessful connection check would have done.

Testing:
All the jdk_jmx and jdk_management regression tests passed.
All the related JCK tests passed.

The fix applies cleanly to 8u and 7u repos.


Thanks,
-JB-

Re: RFR JDK-8031554: com/sun/tools/attach/BasicTests.java fails intermittently

[shanliang]

Jaroslav Bachorik wrote:

On 07/01/2014 11:40 PM, shanliang wrote:

Jaroslav Bachorik wrote:

Hi Shanliang,

On 07/01/2014 09:47 PM, shanliang wrote:

I am still thinking to keep the original fix, because:
1) to throw InterruptedException does not fix the test failure, it 
might

give more info for diagnostics. If it was really caused by an
InterruptedException, then to fix the issue we still need to know who
could interrupt the test main thread, in which case and why, and 
whether

possible to ignore it (skip the test or try again?).


I'm sorry but I can't agree with this. The proposed patch does not add
anything else than making the InterruptedException visible. Adding
process.waitFor() will solve nothing as it is already called by
ProcessTools.getOutput(process) while creating OutputAnalyzer.


2) the test library is used also by other tests and to modify it might
make new fail, it is better to concentrate at first on a single test
before knowing the exact cause.


I wouldn't expect new failures - when an InterruptedException was
thrown the ProcessTools.executeTestJvm(args) returned null. Either the
test would fail with NPE or custom assert - it makes no sense to work
with "null" process.

IMO, this should be fixed in the test library.

Sorry I may miss something here, you suggested:
"Either the result of ProcessTools.getOutput() should be checked for
null to detect this situation or ProcessTools.getOutput() should throw a
more aggressive exception when waitFor() gets interrupted."

We still need to do something when an InterruptedException happens, skip
the test or retry? before making a decision we should know why there was
an InterruptedException and in which case, I really do not have any
idea, and I do not want to exclude other possibilities.


The most probable explanation, based on the analysis of many 
intermittent test failures, is that the harness is trying to time out 
the test. But it is just an educated guess ...
Thought about this possibility, but a harness would kill the test 
instead to interrupt the test thread?




Yes what my fix does is to expose an InterruptedException if it happens,
but it could make sure that it was really because of an
InterruptedException.


I don't feel particularly happy about the code duplication introduced 
by this fix. But if official reviewers are fine with it I won't block 
this review.




About new failure, there could be a negative test  which expected a
IllegalStateException --> failed OutputAnalyser, who knows.


My concern is that there also might be other tests failing 
intermittently (or providing wrong results) due to the 
InterruptedException being silently swallowed.
I agree that we need to do investigation on the test library, but better 
to do it later when we have more info from this test.


Thanks,
Shanliang


You might run the whole testsuite with the modified 
ProcessTools.getOutput() on JPRT and see if there are any negative 
tests failing (they should since instead of null value they will 
receive InterryptedException).


-JB-



Shanliang


-JB-



Shanliang

Christian Tornqvist wrote:


I can’t remember if there was a reason for doing it like this, but it
seems reasonable to not catch the InterruptedException in 
getOutput().


Thanks,

Christian

*From:*Staffan Larsen [mailto:staffan.lar...@oracle.com]
*Sent:* Friday, June 27, 2014 4:49 AM
*To:* shanliang
*Cc:* Jaroslav Bachorik; serviceability-dev@openjdk.java.net
serviceability-dev@openjdk.java.net; Christian Tornqvist
*Subject:* Re: RFR JDK-8031554: com/sun/tools/attach/BasicTests.java
fails intermittently

It does look suspicious to catch and ignore the InterruptedException,
especially since the OutputAnalyzer constructor will fail in this 
case.


Christian is the original author of these classes: do you know if
there is a good rationale for doing this? Or should we propagate the
InterruptedException?

Thanks,

/Staffan

On 26 jun 2014, at 17:24, shanliang mailto:shanliang.ji...@oracle.com>> wrote:



Jaroslav Bachorik wrote:

    Hi Shanliang,

On 06/26/2014 03:15 PM, shanliang wrote:

Hi,

Today ProcessTools.executeProcess has the code:
new OutputAnalyzer(pb.start());

and OutputAnalyzer constructor calls immediately:
exitValue = process.exitValue();

the test got exception because the process did not end.


Are you sure about this?

The OutputAnalyzer constructor, before calling
process.exitValue(), calls ProcessTools.getOutput(process)
which actually does process.waitFor()

Good catch!


A probable explanation would be that process.waitFor() gets
interrupted without the target process actually ending.

Either the result of ProcessTools.getOutput() should be
checked for null to detect this situation or
ProcessTools.getOutput() should throw a

Re: RFR JDK-8031554: com/sun/tools/attach/BasicTests.java fails intermittently

[shanliang]

Jaroslav Bachorik wrote:

Hi Shanliang,

On 07/01/2014 09:47 PM, shanliang wrote:

I am still thinking to keep the original fix, because:
1) to throw InterruptedException does not fix the test failure, it might
give more info for diagnostics. If it was really caused by an
InterruptedException, then to fix the issue we still need to know who
could interrupt the test main thread, in which case and why, and whether
possible to ignore it (skip the test or try again?).


I'm sorry but I can't agree with this. The proposed patch does not add 
anything else than making the InterruptedException visible. Adding 
process.waitFor() will solve nothing as it is already called by 
ProcessTools.getOutput(process) while creating OutputAnalyzer.



2) the test library is used also by other tests and to modify it might
make new fail, it is better to concentrate at first on a single test
before knowing the exact cause.


I wouldn't expect new failures - when an InterruptedException was 
thrown the ProcessTools.executeTestJvm(args) returned null. Either the 
test would fail with NPE or custom assert - it makes no sense to work 
with "null" process.


IMO, this should be fixed in the test library.

Sorry I may miss something here, you suggested:
   "Either the result of ProcessTools.getOutput() should be checked for 
null to detect this situation or ProcessTools.getOutput() should throw a 
more aggressive exception when waitFor() gets interrupted."


We still need to do something when an InterruptedException happens, skip 
the test or retry? before making a decision we should know why there was 
an InterruptedException and in which case, I really do not have any 
idea, and I do not want to exclude other possibilities.


Yes what my fix does is to expose an InterruptedException if it happens, 
but it could make sure that it was really because of an 
InterruptedException.


About new failure, there could be a negative test  which expected a 
IllegalStateException --> failed OutputAnalyser, who knows.


Shanliang


-JB-



Shanliang

Christian Tornqvist wrote:


I can’t remember if there was a reason for doing it like this, but it
seems reasonable to not catch the InterruptedException in getOutput().

Thanks,

Christian

*From:*Staffan Larsen [mailto:staffan.lar...@oracle.com]
*Sent:* Friday, June 27, 2014 4:49 AM
*To:* shanliang
*Cc:* Jaroslav Bachorik; serviceability-dev@openjdk.java.net
serviceability-dev@openjdk.java.net; Christian Tornqvist
*Subject:* Re: RFR JDK-8031554: com/sun/tools/attach/BasicTests.java
fails intermittently

It does look suspicious to catch and ignore the InterruptedException,
especially since the OutputAnalyzer constructor will fail in this case.

Christian is the original author of these classes: do you know if
there is a good rationale for doing this? Or should we propagate the
InterruptedException?

Thanks,

/Staffan

On 26 jun 2014, at 17:24, shanliang mailto:shanliang.ji...@oracle.com>> wrote:



Jaroslav Bachorik wrote:

Hi Shanliang,

On 06/26/2014 03:15 PM, shanliang wrote:

Hi,

Today ProcessTools.executeProcess has the code:
new OutputAnalyzer(pb.start());

and OutputAnalyzer constructor calls immediately:
exitValue = process.exitValue();

the test got exception because the process did not end.


Are you sure about this?

The OutputAnalyzer constructor, before calling
process.exitValue(), calls ProcessTools.getOutput(process)
which actually does process.waitFor()

Good catch!


A probable explanation would be that process.waitFor() gets
interrupted without the target process actually ending.

Either the result of ProcessTools.getOutput() should be
checked for null to detect this situation or
ProcessTools.getOutput() should throw a more aggressive
exception when waitFor() gets interrupted.

It was possible beacause of an InterruptedException, but maybe a
Process issue too.

process.waitFor() was called by the test main thread, I am
wondering who wanted to interrupt this thread?

Not know why ProcessTools.getOutput() catches InterruptedException
and then returns null, are there some other tests dependent to
this behavior? otherwise better to not catch InterruptedException.

I think to keep this modification, it will allow us to get more
information if the bug is reproduced, if getting an
InterruptedException then we need to do more investigation for
why, if no exception then we may rise a question to 
process.waitFor().


Shanliang


-JB-



So a direct solution for the test is not to call:
   ProcessTools.executeTestJvm(args);

but:
ProcessBuilder pb =

ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(args));

  

Re: RFR JDK-8031554: com/sun/tools/attach/BasicTests.java fails intermittently

[shanliang]

I am still thinking to keep the original fix, because:
1) to throw InterruptedException does not fix the test failure, it might 
give more info for diagnostics. If it was really caused by an 
InterruptedException, then to fix the issue we still need to know who 
could interrupt the test main thread, in which case and why, and whether 
possible to ignore it (skip the test or try again?).
2) the test library is used also by other tests and to modify it might 
make new fail, it is better to concentrate at first on a single test 
before knowing the exact cause.


Shanliang

Christian Tornqvist wrote:


I can't remember if there was a reason for doing it like this, but it 
seems reasonable to not catch the InterruptedException in getOutput().


 


Thanks,

Christian

 


*From:* Staffan Larsen [mailto:staffan.lar...@oracle.com]
*Sent:* Friday, June 27, 2014 4:49 AM
*To:* shanliang
*Cc:* Jaroslav Bachorik; serviceability-dev@openjdk.java.net 
serviceability-dev@openjdk.java.net; Christian Tornqvist
*Subject:* Re: RFR JDK-8031554: com/sun/tools/attach/BasicTests.java 
fails intermittently


 

It does look suspicious to catch and ignore the InterruptedException, 
especially since the OutputAnalyzer constructor will fail in this case. 

 

Christian is the original author of these classes: do you know if 
there is a good rationale for doing this? Or should we propagate the 
InterruptedException?


 


Thanks,

/Staffan

 

On 26 jun 2014, at 17:24, shanliang <mailto:shanliang.ji...@oracle.com>> wrote:




Jaroslav Bachorik wrote:

    Hi Shanliang,

On 06/26/2014 03:15 PM, shanliang wrote:

Hi,

Today ProcessTools.executeProcess has the code:
new OutputAnalyzer(pb.start());

and OutputAnalyzer constructor calls immediately:
exitValue = process.exitValue();

the test got exception because the process did not end.


Are you sure about this?

The OutputAnalyzer constructor, before calling
process.exitValue(), calls ProcessTools.getOutput(process)
which actually does process.waitFor()

Good catch!


A probable explanation would be that process.waitFor() gets
interrupted without the target process actually ending.

Either the result of ProcessTools.getOutput() should be
checked for null to detect this situation or
ProcessTools.getOutput() should throw a more aggressive
exception when waitFor() gets interrupted.

It was possible beacause of an InterruptedException, but maybe a
Process issue too.

process.waitFor() was called by the test main thread, I am
wondering who wanted to interrupt this thread?

Not know why ProcessTools.getOutput() catches InterruptedException
and then returns null, are there some other tests dependent to
this behavior? otherwise better to not catch InterruptedException.

I think to keep this modification, it will allow us to get more
information if the bug is reproduced, if getting an
InterruptedException then we need to do more investigation for
why, if no exception then we may rise a question to process.waitFor().

Shanliang


-JB-



So a direct solution for the test is not to call:
   ProcessTools.executeTestJvm(args);

but:
ProcessBuilder pb =
ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(args));
Process process = pb.start();
process.waitFor();
OutputAnalyzer output = new OutputAnalyzer(process);

here we do waiting:
process.waitFor();
before constructing an OutputAnalyzer.

ProcessTools is a tool class we may have same issue for
other tests
using this class. So we may need to improve the test library.

bug: https://bugs.openjdk.java.net/browse/JDK-8031554
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8031554/00/
<http://cr.openjdk.java.net/%7Esjiang/JDK-8031554/00/>


Thanks,
    Shanliang

 

Re: RFR JDK-8031554: com/sun/tools/attach/BasicTests.java fails intermittently

[shanliang]

Jaroslav Bachorik wrote:

Hi Shanliang,

On 06/26/2014 03:15 PM, shanliang wrote:

Hi,

Today ProcessTools.executeProcess has the code:
 new OutputAnalyzer(pb.start());

and OutputAnalyzer constructor calls immediately:
 exitValue = process.exitValue();

the test got exception because the process did not end.


Are you sure about this?

The OutputAnalyzer constructor, before calling process.exitValue(), 
calls ProcessTools.getOutput(process) which actually does 
process.waitFor()

Good catch!


A probable explanation would be that process.waitFor() gets 
interrupted without the target process actually ending.


Either the result of ProcessTools.getOutput() should be checked for 
null to detect this situation or ProcessTools.getOutput() should throw 
a more aggressive exception when waitFor() gets interrupted.
It was possible beacause of an InterruptedException, but maybe a Process 
issue too.


process.waitFor() was called by the test main thread, I am wondering who 
wanted to interrupt this thread?


Not know why ProcessTools.getOutput() catches InterruptedException and 
then returns null, are there some other tests dependent to this 
behavior? otherwise better to not catch InterruptedException.


I think to keep this modification, it will allow us to get more 
information if the bug is reproduced, if getting an InterruptedException 
then we need to do more investigation for why, if no exception then we 
may rise a question to process.waitFor().


Shanliang


-JB-



So a direct solution for the test is not to call:
ProcessTools.executeTestJvm(args);

but:
 ProcessBuilder pb =
ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(args));
 Process process = pb.start();
 process.waitFor();
 OutputAnalyzer output = new OutputAnalyzer(process);

here we do waiting:
 process.waitFor();
before constructing an OutputAnalyzer.

ProcessTools is a tool class we may have same issue for other tests
using this class. So we may need to improve the test library.

bug: https://bugs.openjdk.java.net/browse/JDK-8031554
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8031554/00/


Thanks,
Shanliang

RFR JDK-8031554: com/sun/tools/attach/BasicTests.java fails intermittently

[shanliang]

Hi,

Today ProcessTools.executeProcess has the code:
new OutputAnalyzer(pb.start());

and OutputAnalyzer constructor calls immediately:
exitValue = process.exitValue();

the test got exception because the process did not end.

So a direct solution for the test is not to call:
   ProcessTools.executeTestJvm(args);

but:
ProcessBuilder pb = 
ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(args));

Process process = pb.start();
process.waitFor();
OutputAnalyzer output = new OutputAnalyzer(process);

here we do waiting:
process.waitFor();
before constructing an OutputAnalyzer.

ProcessTools is a tool class we may have same issue for other tests 
using this class. So we may need to improve the test library.


bug: https://bugs.openjdk.java.net/browse/JDK-8031554
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8031554/00/


Thanks,
Shanliang

RFR 8038321: RMIConnector_NPETest.java can't start rmid if running on fastdebug or Solaris-sparc

[shanliang]

Hi,

The test is to test a NPE during creating a JMX RMI server, but it got 
timeout when starting a RMID, the fix is to remove this timeout, it is 
not easy to have a reasonable timeout working for different testing 
machines, and the test is not implemented to test sun.rmi.server.Activation.


bug: https://bugs.openjdk.java.net/browse/JDK-8038321
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8038321/00/ 
<http://cr.openjdk.java.net/%7Esjiang/JDK-8038321/00/>


Thanks,
Shanliang

Codereview request: JDK-8044865 Fix raw and unchecked lint warnings in management-related code

[shanliang]

Hi,

Please review the following fix:

webrev: http://cr.openjdk.java.net/~sjiang/JDK-8044865/00/ 
<http://cr.openjdk.java.net/%7Esjiang/JDK-8044865/00/>

bug: https://bugs.openjdk.java.net/browse/JDK-8044865

Thanks,
Shanliang

Re: RFR 8038322: CounterMonitorDeadlockTest.java fails intermittently, presumed deadlock

[shanliang]

Daniel Fuchs wrote:

Hi Shanliang,

This looks reasonable to me.
Another possibility could have been to use the timeout factor
to adapt the delay of Thread.sleep in the loop.
Yes we could adapt our timeout, but it is simpler to use the testing 
harness timeout.


What you have might be more reliable, but it also means
that if the test ever fails in timeout again then it could
be more difficult to diagnose what was wrong.

Maybe you should add a trace before entering
and after leaving any of your two loops, so that you
at least will know where the test was being blocked?

Yes, absolutely useful to add trace:
http://cr.openjdk.java.net/~sjiang/JDK-8038322/01/ 
<http://cr.openjdk.java.net/%7Esjiang/JDK-8038322/01/>


Thanks,
Shanliang


best regards,

-- daniel

On 5/2/14 11:17 AM, shanliang wrote:

Please review this test fix.

Bug: https://bugs.openjdk.java.net/browse/JDK-8038322
Webrev: http://cr.openjdk.java.net/~sjiang/JDK-8038322/00/
<http://cr.openjdk.java.net/%7Esjiang/JDK-8038322/00/>

The test had a timeout of 500*10 ms, better to remove this timeout and
to wait the harness timeout.

Thanks,
Shanliang

RFR 8038322: CounterMonitorDeadlockTest.java fails intermittently, presumed deadlock

[shanliang]

Please review this test fix.

Bug: https://bugs.openjdk.java.net/browse/JDK-8038322
Webrev: http://cr.openjdk.java.net/~sjiang/JDK-8038322/00/ 
<http://cr.openjdk.java.net/%7Esjiang/JDK-8038322/00/>


The test had a timeout of 500*10 ms, better to remove this timeout and 
to wait the harness timeout.


Thanks,
Shanliang

Re: RFR: 8039432 demo/jvmti/mtrace/TraceJFrame.java can't connect to X11

[shanliang]

Staffan Larsen wrote:


On 9 apr 2014, at 10:24, shanliang <mailto:shanliang.ji...@oracle.com>> wrote:



Staffan Larsen wrote:

On 8 apr 2014, at 22:10, shanliang  wrote:

  

Hi Staffan,

I did work on this bug and I thought the problem was from a bad DISPLAY setting 
(see my comments in the bug), I might miss something here.

I looked at the class GraphicsEnvironment.java, getLocalGraphicsEnvironment() 
calls createGE(), and the latter has the following code:

  Class geCls;
   ..
  ge = geCls.newInstance();
//  long t1 = System.currentTimeMillis();
//  System.out.println("GE creation took " + (t1-t0)+ "ms.");
  if (isHeadless()) {
  ge = new HeadlessGraphicsEnvironment(ge);
  }

so we should not get an AWTError in case of headless, instead we get a 
HeadlessGraphicsEnvironment.


If you look at the exception you can see that it is Class.forName() inside 
createGE() that throws the AWTError. We never get to the if(isHeadless()) 
statement.
  
Yes the exception was thrown before calling isHeadless(), does this 
mean a possible bug in the createGE implementation? should we check 
isHeadLess() before calling Class.forName() ?
 
I repeated my old test, it did call GraphicsEnvironment.isHeadless():

---
public class Test extends JFrame {
public Test() {
   setTitle("Simple example");
   setSize(300, 200);
   setLocationRelativeTo(null);
   setDefaultCloseOperation(EXIT_ON_CLOSE);
}

public static void main(String[] args) {
if (GraphicsEnvironment.isHeadless()) {
System.out.println("JFrame test was skipped due to 
headless mode");


System.exit(0);
}

SwingUtilities.invokeLater(new Runnable() {
@Override
public void run() {
Test ex = new Test();
ex.setVisible(true);
}
});
}
}
---
When running the test with X11 and a wrong DISPLAY, isHeadless did 
not return "true" to stop the test, the test continued and failed 
later with the exception:
  Exception in thread "main" java.lang.InternalError: Can't connect 
to X11 window server using 'toto' as the value of the DISPLAY variable.

at sun.awt.X11GraphicsEnvironment.initDisplay(Native Method)
at 
sun.awt.X11GraphicsEnvironment.access$200(X11GraphicsEnvironment.java:65)
at 
sun.awt.X11GraphicsEnvironment$1.run(X11GraphicsEnvironment.java:110)

at java.security.AccessController.doPrivileged(Native Method)
at 
sun.awt.X11GraphicsEnvironment.(X11GraphicsEnvironment.java:74)

at java.lang.Class.forName0(Native Method)
at java.lang.Class.forName(Class.java:190)
at 
java.awt.GraphicsEnvironment.createGE(GraphicsEnvironment.java:102)
at 
java.awt.GraphicsEnvironment.getLocalGraphicsEnvironment(GraphicsEnvironment.java:81)

   ...

I am wondering that "isHeadLess" would not check a wrong DISPLAY, a 
quick look at the method implementation seems telling yes.


It assumes that if you have DISPLAY set, that it is the correct value. 
Which is a fair assumption, I think.
Yes we do have to set DISPLAY for this issue, I set a wrong DISPLAY to 
reproduce the bug.


With my change, the test now passes in JPRT which it did not before.

Good to hear that the test passes!

Thanks,
Shanliang


/Staffan



Shanliang

/Staffan

  

Look at the following 2 methods:

  public static boolean isHeadless() {
  return getHeadlessProperty();
  }

  public boolean isHeadlessInstance() {
  // By default (local graphics environment), simply check the
  // headless property.
  return getHeadlessProperty();
  }

it seems no difference to call
  GraphicsEnvironment.getLocalGraphicsEnvironement().isHeadlessInstance()
and
  GraphicsEnvironment.isHeadless()   
yes better to cal the static method, but I am not sure that the direct call would fix the failure.


Thanks,
Shanliang


Staffan Larsen wrote:


This test causes exceptions that looks like this:

java.awt.AWTError: Can't connect to X11 window server using ‘REDACTED:503' as 
the value of the DISPLAY variable.
at sun.awt.X11GraphicsEnvironment.initDisplay(Native Method)
at 
sun.awt.X11GraphicsEnvironment.access$200(X11GraphicsEnvironment.java:65)
at sun.awt.X11GraphicsEnvironment$1.run(X11GraphicsEnvironment.java:115)
at java.security.AccessController.doPrivileged(Native Method)
at 
sun.awt.X11GraphicsEnvironment.(X11GraphicsEnvironment.java:74)
at java.lang.Class.forName0(Native Method)
at java.lang.Class.forName(Class.java:259)
at java.awt.GraphicsEnvironment.createGE(GraphicsEnvironment.java:102)
at 
java.awt.GraphicsEnvironment.getLocalGraphicsEnvironment(GraphicsEnvironment.java:81)
   

Re: RFR: 8039432 demo/jvmti/mtrace/TraceJFrame.java can't connect to X11

[shanliang]

Staffan Larsen wrote:

On 8 apr 2014, at 22:10, shanliang  wrote:

  

Hi Staffan,

I did work on this bug and I thought the problem was from a bad DISPLAY setting 
(see my comments in the bug), I might miss something here.

I looked at the class GraphicsEnvironment.java, getLocalGraphicsEnvironment() 
calls createGE(), and the latter has the following code:

  Class geCls;
   ..
  ge = geCls.newInstance();
//  long t1 = System.currentTimeMillis();
//  System.out.println("GE creation took " + (t1-t0)+ "ms.");
  if (isHeadless()) {
  ge = new HeadlessGraphicsEnvironment(ge);
  }

so we should not get an AWTError in case of headless, instead we get a 
HeadlessGraphicsEnvironment.



If you look at the exception you can see that it is Class.forName() inside 
createGE() that throws the AWTError. We never get to the if(isHeadless()) 
statement.
  
Yes the exception was thrown before calling isHeadless(), does this mean 
a possible bug in the createGE implementation? should we check 
isHeadLess() before calling Class.forName() ?


I repeated my old test, it did call GraphicsEnvironment.isHeadless():
---
public class Test extends JFrame {
   public Test() {
  setTitle("Simple example");
  setSize(300, 200);
  setLocationRelativeTo(null);
  setDefaultCloseOperation(EXIT_ON_CLOSE);
   }

   public static void main(String[] args) {
   if (GraphicsEnvironment.isHeadless()) {
   System.out.println("JFrame test was skipped due to headless 
mode");


   System.exit(0);
   }

   SwingUtilities.invokeLater(new Runnable() {
   @Override
   public void run() {
   Test ex = new Test();
   ex.setVisible(true);
   }
   });
   }
}
---
When running the test with X11 and a wrong DISPLAY, isHeadless did not 
return "true" to stop the test, the test continued and failed later with 
the exception:
 Exception in thread "main" java.lang.InternalError: Can't connect to 
X11 window server using 'toto' as the value of the DISPLAY variable.

   at sun.awt.X11GraphicsEnvironment.initDisplay(Native Method)
   at 
sun.awt.X11GraphicsEnvironment.access$200(X11GraphicsEnvironment.java:65)
   at 
sun.awt.X11GraphicsEnvironment$1.run(X11GraphicsEnvironment.java:110)

   at java.security.AccessController.doPrivileged(Native Method)
   at 
sun.awt.X11GraphicsEnvironment.(X11GraphicsEnvironment.java:74)

   at java.lang.Class.forName0(Native Method)
   at java.lang.Class.forName(Class.java:190)
   at 
java.awt.GraphicsEnvironment.createGE(GraphicsEnvironment.java:102)
   at 
java.awt.GraphicsEnvironment.getLocalGraphicsEnvironment(GraphicsEnvironment.java:81)

  ...

I am wondering that "isHeadLess" would not check a wrong DISPLAY, a 
quick look at the method implementation seems telling yes.


Shanliang

/Staffan

  

Look at the following 2 methods:

  public static boolean isHeadless() {
  return getHeadlessProperty();
  }

  public boolean isHeadlessInstance() {
  // By default (local graphics environment), simply check the
  // headless property.
  return getHeadlessProperty();
  }

it seems no difference to call
  GraphicsEnvironment.getLocalGraphicsEnvironement().isHeadlessInstance()
and
  GraphicsEnvironment.isHeadless()   
yes better to cal the static method, but I am not sure that the direct call would fix the failure.


Thanks,
Shanliang


Staffan Larsen wrote:


This test causes exceptions that looks like this:

java.awt.AWTError: Can't connect to X11 window server using ‘REDACTED:503' as 
the value of the DISPLAY variable.
at sun.awt.X11GraphicsEnvironment.initDisplay(Native Method)
at 
sun.awt.X11GraphicsEnvironment.access$200(X11GraphicsEnvironment.java:65)
at sun.awt.X11GraphicsEnvironment$1.run(X11GraphicsEnvironment.java:115)
at java.security.AccessController.doPrivileged(Native Method)
at 
sun.awt.X11GraphicsEnvironment.(X11GraphicsEnvironment.java:74)
at java.lang.Class.forName0(Native Method)
at java.lang.Class.forName(Class.java:259)
at java.awt.GraphicsEnvironment.createGE(GraphicsEnvironment.java:102)
at 
java.awt.GraphicsEnvironment.getLocalGraphicsEnvironment(GraphicsEnvironment.java:81)
at TraceJFrame.main(TraceJFrame.java:39)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:484)
at 
com.sun.javatest.regtest.MainAction$SameVMRunnable.run(MainAction.java:754)
at java.lang.Thread.run(Threa

Re: RFR: 8039432 demo/jvmti/mtrace/TraceJFrame.java can't connect to X11

[shanliang]

Hi Staffan,

I did work on this bug and I thought the problem was from a bad DISPLAY 
setting (see my comments in the bug), I might miss something here.


I looked at the class GraphicsEnvironment.java, 
getLocalGraphicsEnvironment() calls createGE(), and the latter has the 
following code:


   Class geCls;
..
   ge = geCls.newInstance();
//  long t1 = System.currentTimeMillis();
//  System.out.println("GE creation took " + (t1-t0)+ "ms.");
   if (isHeadless()) {
   ge = new HeadlessGraphicsEnvironment(ge);
   }

so we should not get an AWTError in case of headless, instead we get a 
HeadlessGraphicsEnvironment.


Look at the following 2 methods:

   public static boolean isHeadless() {
   return getHeadlessProperty();
   }

   public boolean isHeadlessInstance() {
   // By default (local graphics environment), simply check the
   // headless property.
   return getHeadlessProperty();
   }

it seems no difference to call
   GraphicsEnvironment.getLocalGraphicsEnvironement().isHeadlessInstance()
and
   GraphicsEnvironment.isHeadless()   

yes better to cal the static method, but I am not sure that the direct 
call would fix the failure.


Thanks,
Shanliang


Staffan Larsen wrote:

This test causes exceptions that looks like this:

java.awt.AWTError: Can't connect to X11 window server using ‘REDACTED:503' as 
the value of the DISPLAY variable.
at sun.awt.X11GraphicsEnvironment.initDisplay(Native Method)
at 
sun.awt.X11GraphicsEnvironment.access$200(X11GraphicsEnvironment.java:65)
at sun.awt.X11GraphicsEnvironment$1.run(X11GraphicsEnvironment.java:115)
at java.security.AccessController.doPrivileged(Native Method)
at 
sun.awt.X11GraphicsEnvironment.(X11GraphicsEnvironment.java:74)
at java.lang.Class.forName0(Native Method)
at java.lang.Class.forName(Class.java:259)
at java.awt.GraphicsEnvironment.createGE(GraphicsEnvironment.java:102)
at 
java.awt.GraphicsEnvironment.getLocalGraphicsEnvironment(GraphicsEnvironment.java:81)
at TraceJFrame.main(TraceJFrame.java:39)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:484)
at 
com.sun.javatest.regtest.MainAction$SameVMRunnable.run(MainAction.java:754)
at java.lang.Thread.run(Thread.java:744)


The fix seems to be to not call 
GraphicsEnvironment.getLocalGraphicsEnvironement().isHeadlessInstance() but 
GraphicsEnvironment.isHeadless() directly.

Please review the fix below,

Thanks,
/Staffan



diff --git a/test/demo/jvmti/mtrace/TraceJFrame.java 
b/test/demo/jvmti/mtrace/TraceJFrame.java
--- a/test/demo/jvmti/mtrace/TraceJFrame.java
+++ b/test/demo/jvmti/mtrace/TraceJFrame.java
@@ -36,7 +36,7 @@

 public class TraceJFrame {
 public static void main(String args[]) throws Exception {
-if 
(GraphicsEnvironment.getLocalGraphicsEnvironment().isHeadlessInstance()) {
+if (GraphicsEnvironment.isHeadless()) {
 System.out.println("JFrame test was skipped due to headless mode");
 } else {
 DemoRun demo;

Re: RFR 8039080: "jinfo server_id@host" fails with "Invalid process identifier"

[shanliang]

Jaroslav Bachorik wrote:

On 7.4.2014 14:10, shanliang wrote:

Jaroslav,

Is it necessary to add "ValidationException"?
Well, for the implementation when the validation is performed at the 
moment a new instance of JInfo is being created it is necessary.
Could not use an existing Exception? like IllegalArgumentException? I 
did not see your constructor throw a same exception.




Could we change the constructor JInfo to:
private static boolean validateArgs(String[] args);
the method returns false if the args are illegal, or throw an
IllegalArgumentException

and declare the variables "args" and "useSA" as static too,


Yes, of course this could be changed like this. What would be the 
expected benefits?

The code will be simpler with one less inner class

Shanliang


-JB-



Shanliang

Jaroslav Bachorik wrote:

Hi,

Sorry for the noise but I need to get the fix re-reviewed.
Due to the way jtreg cooperates with TestNG when runnning in agentvm I
can not use package private methods or constructor or fields.

The updated patch -
http://cr.openjdk.java.net/~jbachorik/8039080/webrev.01 - makes the
JInfo constructor a private one and removes the package private
getters. The test is using reflection to create new instances of JInfo
and to assert its state.

Thanks,

-JB-

Re: RFR 8039080: "jinfo server_id@host" fails with "Invalid process identifier"

[shanliang]

shanliang wrote:

Jaroslav,

Is it necessary to add "ValidationException"?

Could we change the constructor JInfo to:
   private static boolean validateArgs(String[] args);
the method returns false if the args are illegal, or throw an 
IllegalArgumentException


and declare the variables "args" and "useSA" as static too,
Static variables may have problem if called with multi-thread, but we 
still could do:


private static Map validate(String[] args) throws 
IllegalArgumentException;


the return map contains args(String[]) and useSA(boolean).

Or put "USE_SA" as a new arg into the args list, then the method becomes:
   private static String[] validate(String[] args) throws 
IllegalArgumentException;


Shanliang



Shanliang

Jaroslav Bachorik wrote:

Hi,

Sorry for the noise but I need to get the fix re-reviewed.
Due to the way jtreg cooperates with TestNG when runnning in agentvm 
I can not use package private methods or constructor or fields.


The updated patch - 
http://cr.openjdk.java.net/~jbachorik/8039080/webrev.01 - makes the 
JInfo constructor a private one and removes the package private 
getters. The test is using reflection to create new instances of 
JInfo and to assert its state.


Thanks,

-JB-

Re: RFR 8039080: "jinfo server_id@host" fails with "Invalid process identifier"

[shanliang]

Jaroslav,

Is it necessary to add "ValidationException"?

Could we change the constructor JInfo to:
   private static boolean validateArgs(String[] args);
the method returns false if the args are illegal, or throw an 
IllegalArgumentException


and declare the variables "args" and "useSA" as static too,

Shanliang

Jaroslav Bachorik wrote:

Hi,

Sorry for the noise but I need to get the fix re-reviewed.
Due to the way jtreg cooperates with TestNG when runnning in agentvm I 
can not use package private methods or constructor or fields.


The updated patch - 
http://cr.openjdk.java.net/~jbachorik/8039080/webrev.01 - makes the 
JInfo constructor a private one and removes the package private 
getters. The test is using reflection to create new instances of JInfo 
and to assert its state.


Thanks,

-JB-

Re: RFR: 6815126 intermittent SimulResumerTest.java failure

[shanliang]

Hope to get reviewed and to push this fix:

1) this is a fix for a bug labeled with "svc-nightly"

2) The current test must be useful. Yes the test could not be 100% sure 
to test the bug JDK-6751643, but with its 2*1 resume repeatings it 
would have big chance to hit the bug conditions, the failure the patch 
to fix happened exactly in the condition the bug JDK-6751643 could happen.


3) there is possibly someway to realize the synchronization logic 
between the thread invoking the operations and the thread resuming, I 
could see to add code into the method "resume" to do waiting for this 
test, but I could not see an easy and practical way to do that.


4) we can create a new bug to fix this synchronization issue if necessary.

Thanks,
Shanliang

shanliang wrote:

Jaroslav Bachorik wrote:

Thanks Shanliang, it is clear now.

The patch will get rid off the IOOBE but I have my doubts about what 
the test actually tests. It is supposed to make sure that certain 
operations will not throw NPE when the debugged thread is resumed 
(from a concurrent debugger thread) before the operation has managed 
to finish. However, there seems to be no synchronization logic 
between the thread invoking the operations and the thread resuming 
the paused debugged thread, relying only on hitting this condition by 
chance.


This test seems to be a good candidate for a thorough revision/rewrite.
Not sure how to make the checking happen during a "resuming" window, 
the test creates 2 threads and each repeats "resume"1 times, and 
one another thread repeats checking with 100ms sleeping time,  just 
hoping some checking would fail into a resuming window.


Shanliang


-JB-

On 31.3.2014 11:26, shanliang wrote:

Erik Gahlin wrote:

I also like to understand better.

Possibly my previous reply was not clear enough or I missed something
there.

The test was to test JDK-6751643 as I cited in the last mail, here is
the info from JDK-6751643 to which this test was developed:
--
This bug can only occur if a debugger has multiple threads and calls 
any
of the following methods in one thread while simultaneously resuming 
the
same debuggee thread in a different debugger thread. Debuggers 
shouldn't

do this because it is a race condition and the result returned by these
methods will vary depending upon just where in the processing of these
methods the resume takes effect. EG, the frameCount() method could
return 6 in a case where the debuggee has already been resumed and 
there

are no frames.
--

To reproduce the bug, test did mainly 2 things by different threads:
1) received vm events and resumed vm, this was done by thread 
"Thread-1"

in the class TestScaffold which registered a listener and called the
following method:
/**
 * Events handled directly by scaffold always resume (well, almost
always)
 */
public void eventSetComplete(EventSet set) {
// The listener in connect(..) resumes after receiving our
// special VMDeathEvent.  We can't also do the resume
// here or we will probably get a VMDisconnectedException
if (!containsOurVMDeathRequest(set)) {
traceln("TS: set.resume() called");
set.resume();
}
  }

2) called the method "check" in the class SimulResumerTarg, to see
whether a NullPointerException was thrown, the thread name was "test
resumer" (better to named as "checking thread"?)

So one thread was doing resume, another thread was doing check at same.
I added the code to see the different values of  frames.size() at 
line 185:

for (i=0; i<10:i++) {
System.out.println("---frames.size(): "+frames.size());
Thhread.sleep(200);
}

if printing out frames, sometime we could see one more frame:
------ java.lang.Thread.yield()+-1 in thread 
instance of

SimulResumerTarg(name='Thread 2', id=109)


Shanliang


I looked at this failure before and I couldn't see what was wrong, not
in the test or product.

Erik

Jaroslav Bachorik skrev 3/27/14 4:49 PM:

On 27.3.2014 15:49, shanliang wrote:

Hi,

The call
thr.frames(0, frames.size() - 1);
suffers a synchronization issue, the size may be changed after
frames.size() returns.


Any idea why there is a synchronization issue? The code seems to be
intended to run only when a breakpoint is hit and the target thread
is suspended.

-JB-



webrev:
http://cr.openjdk.java.net/~sjiang/JDK-6815126/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-6815126

Shanliang

RFR: 8038940 c.s.j.r.i.ClientNotifForwarder$LinearExecutor prone to data races

[shanliang]

Hi,

When ClientNotifForwarder starts, its first communication with 
ServerNotifForwarder is to get clientSequenceNumber, then starts 
LinearExecutor to execute the fetching job. If reconnection happens 
during this communication, a new thread will be started by the 
reconnection to do fetching job too, that's why the test got 
"java.lang.IllegalArgumentException: More than one command "


I have verified the class ClientNotifForwarder to make sure no other 
place would start a new job.


It is difficult to have a regression test to reproduce this bug, it is 
all related to an internal function. I had to add code temporally into 
the implementation to make the reconnection happen during this first 
communication, in order to reproduce the bug and to verify the fix.


webrev: http://cr.openjdk.java.net/~sjiang/JDK-8038940/00/
bug: https://bugs.openjdk.java.net/browse/JDK-8038940

Thanks,
Shanliang

Re: RFR: 6815126 intermittent SimulResumerTest.java failure

[shanliang]

Jaroslav Bachorik wrote:

Thanks Shanliang, it is clear now.

The patch will get rid off the IOOBE but I have my doubts about what 
the test actually tests. It is supposed to make sure that certain 
operations will not throw NPE when the debugged thread is resumed 
(from a concurrent debugger thread) before the operation has managed 
to finish. However, there seems to be no synchronization logic between 
the thread invoking the operations and the thread resuming the paused 
debugged thread, relying only on hitting this condition by chance.


This test seems to be a good candidate for a thorough revision/rewrite.
Not sure how to make the checking happen during a "resuming" window, the 
test creates 2 threads and each repeats "resume"1 times, and one 
another thread repeats checking with 100ms sleeping time,  just hoping 
some checking would fail into a resuming window.


Shanliang


-JB-

On 31.3.2014 11:26, shanliang wrote:

Erik Gahlin wrote:

I also like to understand better.

Possibly my previous reply was not clear enough or I missed something
there.

The test was to test JDK-6751643 as I cited in the last mail, here is
the info from JDK-6751643 to which this test was developed:
--
This bug can only occur if a debugger has multiple threads and calls any
of the following methods in one thread while simultaneously resuming the
same debuggee thread in a different debugger thread. Debuggers shouldn't
do this because it is a race condition and the result returned by these
methods will vary depending upon just where in the processing of these
methods the resume takes effect. EG, the frameCount() method could
return 6 in a case where the debuggee has already been resumed and there
are no frames.
--

To reproduce the bug, test did mainly 2 things by different threads:
1) received vm events and resumed vm, this was done by thread "Thread-1"
in the class TestScaffold which registered a listener and called the
following method:
/**
 * Events handled directly by scaffold always resume (well, almost
always)
 */
public void eventSetComplete(EventSet set) {
// The listener in connect(..) resumes after receiving our
// special VMDeathEvent.  We can't also do the resume
// here or we will probably get a VMDisconnectedException
if (!containsOurVMDeathRequest(set)) {
traceln("TS: set.resume() called");
set.resume();
}
  }

2) called the method "check" in the class SimulResumerTarg, to see
whether a NullPointerException was thrown, the thread name was "test
resumer" (better to named as "checking thread"?)

So one thread was doing resume, another thread was doing check at same.
I added the code to see the different values of  frames.size() at 
line 185:

for (i=0; i<10:i++) {
System.out.println("---frames.size(): "+frames.size());
Thhread.sleep(200);
}

if printing out frames, sometime we could see one more frame:
-- java.lang.Thread.yield()+-1 in thread instance of
SimulResumerTarg(name='Thread 2', id=109)


Shanliang


I looked at this failure before and I couldn't see what was wrong, not
in the test or product.

Erik

Jaroslav Bachorik skrev 3/27/14 4:49 PM:

On 27.3.2014 15:49, shanliang wrote:

Hi,

The call
thr.frames(0, frames.size() - 1);
suffers a synchronization issue, the size may be changed after
frames.size() returns.


Any idea why there is a synchronization issue? The code seems to be
intended to run only when a breakpoint is hit and the target thread
is suspended.

-JB-



webrev:
http://cr.openjdk.java.net/~sjiang/JDK-6815126/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-6815126

Shanliang

Re: RFR: 6815126 intermittent SimulResumerTest.java failure

[shanliang]

Erik Gahlin wrote:

I also like to understand better.

Possibly my previous reply was not clear enough or I missed something there.

The test was to test JDK-6751643 as I cited in the last mail, here is 
the info from JDK-6751643 to which this test was developed:

--
This bug can only occur if a debugger has multiple threads and calls any 
of the following methods in one thread while simultaneously resuming the 
same debuggee thread in a different debugger thread. Debuggers shouldn't 
do this because it is a race condition and the result returned by these 
methods will vary depending upon just where in the processing of these 
methods the resume takes effect. EG, the frameCount() method could 
return 6 in a case where the debuggee has already been resumed and there 
are no frames.

--

To reproduce the bug, test did mainly 2 things by different threads:
1) received vm events and resumed vm, this was done by thread "Thread-1" 
in the class TestScaffold which registered a listener and called the 
following method:

   /**
* Events handled directly by scaffold always resume (well, almost 
always)

*/
   public void eventSetComplete(EventSet set) {
   // The listener in connect(..) resumes after receiving our
   // special VMDeathEvent.  We can't also do the resume
   // here or we will probably get a VMDisconnectedException
   if (!containsOurVMDeathRequest(set)) {
   traceln("TS: set.resume() called");
   set.resume();
   }
 }

2) called the method "check" in the class SimulResumerTarg, to see 
whether a NullPointerException was thrown, the thread name was "test 
resumer" (better to named as "checking thread"?)


So one thread was doing resume, another thread was doing check at same.  
I added the code to see the different values of  frames.size() at line 185:

   for (i=0; i<10:i++) {
   System.out.println("---frames.size(): "+frames.size());
   Thhread.sleep(200);
   }

if printing out frames, sometime we could see one more frame:
   -- java.lang.Thread.yield()+-1 in thread instance of 
SimulResumerTarg(name='Thread 2', id=109)



Shanliang


I looked at this failure before and I couldn't see what was wrong, not 
in the test or product.


Erik

Jaroslav Bachorik skrev 3/27/14 4:49 PM:

On 27.3.2014 15:49, shanliang wrote:

Hi,

The call
thr.frames(0, frames.size() - 1);
suffers a synchronization issue, the size may be changed after
frames.size() returns.


Any idea why there is a synchronization issue? The code seems to be 
intended to run only when a breakpoint is hit and the target thread 
is suspended.


-JB-



webrev:
http://cr.openjdk.java.net/~sjiang/JDK-6815126/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-6815126

Shanliang

Re: RFR: 6815126 intermittent SimulResumerTest.java failure

[shanliang]

Jaroslav Bachorik wrote:

On 27.3.2014 15:49, shanliang wrote:

Hi,

The call
thr.frames(0, frames.size() - 1);
suffers a synchronization issue, the size may be changed after
frames.size() returns.


Any idea why there is a synchronization issue? The code seems to be 
intended to run only when a breakpoint is hit and the target thread is 
suspended.

Here is the info from JDK-6751643 to which this test was developed to test:
--
This bug can only occur if a debugger has multiple threads and calls any 
of the following methods in one thread while simultaneously resuming the 
same debuggee thread in a different debugger thread. Debuggers shouldn't 
do this because it is a race condition and the result returned by these 
methods will vary depending upon just where in the processing of these 
methods the resume takes effect. EG, the frameCount() method could 
return 6 in a case where the debuggee has already been resumed and there 
are no frames.

--

If I do not misunderstand, the check was possibly called before, during 
or after resuming.


Shanliang


-JB-



webrev:
http://cr.openjdk.java.net/~sjiang/JDK-6815126/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-6815126

Shanliang

RFR: 6815126 intermittent SimulResumerTest.java failure

[shanliang]

Hi,

The call
   thr.frames(0, frames.size() - 1);
suffers a synchronization issue, the size may be changed after 
frames.size() returns.


webrev:
http://cr.openjdk.java.net/~sjiang/JDK-6815126/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-6815126

Shanliang

Re: RFR(XS) 8031065: LowMemoryTest2.sh fails: OutOfMemoryError: Metaspace

[shanliang]

Looks good!

It could be improved to not use the variable thresholdExceeded:

change Line 146 to
   while(true) {
and remove Line 143 and 158

Thanks,
Shanliang


Mattias Tobiasson wrote:

Hi,
I have updated the test and now stop allocating when we have reached the 
threshold.
Since we now do all allocations first and then just wait for the notification, 
I have split the loop into two separate loops to make it clearer.

To detect if we have reached the threshold I now check 
MemoryPoolMXBean.getUsageThresholdCount() > 0 instead of checking 
isUsageThresholdExceeded().
The reason for that is because the notification event is not generated 
immediately when isUsageThresholdExceeded() = true. The notification is only 
generated at the next GC. So that is the reason for why the old test kept 
allocating after it had reached the threshold (to trigger another GC).

getUsageThresholdCount() is updated at the same time as the event is generated. So 
after getUsageThresholdCount() > 0, I can just wait for the notification 
without more allocations.

webrev:
http://cr.openjdk.java.net/~mtobiass/8031065/webrev.01

bug:
https://bugs.openjdk.java.net/browse/JDK-8031065

Mattias

- Original Message - 
From: shanliang.ji...@oracle.com 
To: mattias.tobias...@oracle.com 
Cc: serviceability-dev@openjdk.java.net, daniel.fu...@oracle.com 
Sent: Thursday, February 27, 2014 5:12:51 PM GMT +01:00 Amsterdam / Berlin / Bern / Rome / Stockholm / Vienna 
Subject: Re: RFR(XS) 8031065: LowMemoryTest2.sh fails: OutOfMemoryError: Metaspace 



Mattias Tobiasson wrote: 


Hi, thanks for the fast reviews.

I did think about stop calling loadNext() after the flag has been set. The main reason for not doing that was just because I wanted to change as little as possible. Now the test works as originally intended. I prefer to do like this too :) 



I do not mind removing the calls to loadNext(), but then we would need some timeout waiting for the callback. Currently the test "times out" with an OutOfMemory when we have allocated the remaining 20% of the space. You do not need to add a timeout, only change Line 151 
for(;;) 
to 
while(!listenerInvoked) { 

and remove 160 -- 162 

in case that an expected notification is not arrived, the testing harness has a timeout to stop the test. 

This way makes the test more robust, but I am OK with the current fix. 

Thanks, 
Shanliang 



About line 172, you are correct. I will just remove that line. Thanks!

Mattias

- Original Message -
From: shanliang.ji...@oracle.com To: daniel.fu...@oracle.com Cc: 
mattias.tobias...@oracle.com , serviceability-dev@openjdk.java.net Sent: 
Thursday, February 27, 2014 12:59:49 PM GMT +01:00 Amsterdam / Berlin / Bern / 
Rome / Stockholm / Vienna
Subject: Re: RFR(XS) 8031065: LowMemoryTest2.sh fails: OutOfMemoryError: 
Metaspace

Daniel Fuchs wrote: 

On 2/27/14 11:43 AM, Mattias Tobiasson wrote: 


Hi,
Could you please review this test fix.

The test verifies that MemoryPoolMXBean sends a notification when 
used memory has reached the threshold.
The flag thresholdExceeded marks if we have reached the memory 
threshold. When the flag is set, the test slows down to give time for 
the notification to be received.
The problem is that thresholdExceeded is overwritten every time in 
the loop. Instead it should be set if any pool has reached the 
threshold. This means that the test continues to allocate memory at 
full speed, and we may get an OutOfMemory before we get the 
notification. Hi Mattias,


I wonder whether you should also stop calling loadNext() once
thresholdExceeded is true? Yes I am thinking this too.

Line 172 is unnecessary, after thresholdExceeded becomes true, Line 170 
will always be skipped.


Shanliang 


best regards,

-- daniel 

bug: https://bugs.openjdk.java.net/browse/JDK-8031065 webrev: http://cr.openjdk.java.net/~ykantser/8031065/webrev.00/ Mattias 
  

Re: RFR(XS) 8031065: LowMemoryTest2.sh fails: OutOfMemoryError: Metaspace

[shanliang]

Mattias Tobiasson wrote:

Hi, thanks for the fast reviews.

I did think about stop calling loadNext() after the flag has been set. The main 
reason for not doing that was just because I wanted to change as little as 
possible. Now the test works as originally intended.
  

I prefer to do like this too :)

I do not mind removing the calls to loadNext(), but then we would need some timeout 
waiting for the callback. Currently the test "times out" with an OutOfMemory 
when we have allocated the remaining 20% of the space.
  

You do not need to add a timeout, only change Line 151
   for(;;)
to
   while(!listenerInvoked) {

and remove 160 -- 162

in case that an expected notification is not arrived, the testing 
harness has a timeout to stop the test.
  
This way makes the test more robust,  but I am OK with the current fix.


Thanks,
Shanliang


About line 172, you are correct. I will just remove that line. Thanks!

Mattias

- Original Message -
From: shanliang.ji...@oracle.com
To: daniel.fu...@oracle.com
Cc: mattias.tobias...@oracle.com, serviceability-dev@openjdk.java.net
Sent: Thursday, February 27, 2014 12:59:49 PM GMT +01:00 Amsterdam / Berlin / 
Bern / Rome / Stockholm / Vienna
Subject: Re: RFR(XS) 8031065: LowMemoryTest2.sh fails: OutOfMemoryError: 
Metaspace

Daniel Fuchs wrote:
  

On 2/27/14 11:43 AM, Mattias Tobiasson wrote:


Hi,
Could you please review this test fix.

The test verifies that MemoryPoolMXBean sends a notification when 
used memory has reached the threshold.
The flag thresholdExceeded marks if we have reached the memory 
threshold. When the flag is set, the test slows down to give time for 
the notification to be received.
The problem is that thresholdExceeded is overwritten every time in 
the loop. Instead it should be set if any pool has reached the 
threshold. This means that the test continues to allocate memory at 
full speed, and we may get an OutOfMemory before we get the 
notification.


  

Hi Mattias,

I wonder whether you should also stop calling loadNext() once
thresholdExceeded is true?


Yes I am thinking this too.

Line 172 is unnecessary, after thresholdExceeded becomes true, Line 170 
will always be skipped.


Shanliang
  

best regards,

-- daniel



bug:
https://bugs.openjdk.java.net/browse/JDK-8031065

webrev:
http://cr.openjdk.java.net/~ykantser/8031065/webrev.00/

Mattias

  


  

Re: RFR(XS) 8031065: LowMemoryTest2.sh fails: OutOfMemoryError: Metaspace

[shanliang]

Daniel Fuchs wrote:

On 2/27/14 11:43 AM, Mattias Tobiasson wrote:

Hi,
Could you please review this test fix.

The test verifies that MemoryPoolMXBean sends a notification when 
used memory has reached the threshold.
The flag thresholdExceeded marks if we have reached the memory 
threshold. When the flag is set, the test slows down to give time for 
the notification to be received.
The problem is that thresholdExceeded is overwritten every time in 
the loop. Instead it should be set if any pool has reached the 
threshold. This means that the test continues to allocate memory at 
full speed, and we may get an OutOfMemory before we get the 
notification.




Hi Mattias,

I wonder whether you should also stop calling loadNext() once
thresholdExceeded is true?

Yes I am thinking this too.

Line 172 is unnecessary, after thresholdExceeded becomes true, Line 170 
will always be skipped.


Shanliang


best regards,

-- daniel


bug:
https://bugs.openjdk.java.net/browse/JDK-8031065

webrev:
http://cr.openjdk.java.net/~ykantser/8031065/webrev.00/

Mattias

Re: RFR 8035395: sun/management/jmxremote/startstop/JMXStartStopTest.java fails intermittently: Port already in use

[shanliang]

Jaroslav Bachorik wrote:

On 21.2.2014 10:26, shanliang wrote:

Jaroslav Bachorik wrote:

Hi Shanliang,

On 20.2.2014 19:20, shanliang wrote:

Jaroslav,

The failed tests were:
1, 7, 8, 9

but the tests using this port (port2: 50235) were
1, 3, 4, 6, 7, 8, 9

and tests 2,4,6 were passed.

so I think that the problem might be that the port was not fully
released when a test was trying to use it, even the port was closed by
the previous test.


I don't think this is the case.

Firstly, test 1 fails. There are no previous tests possibly holding on
to the required port. So the port must have been taken by some foreign
process. The port number being from the ephemeral range doesn't help
either, quite contrary.

Secondly, a port can not be partially bound - either a process binds
to a port or not. Also, when a process exits all the bound ports must
be released. Since we are waiting for the exit code of the launched
test application before proceeding all the ports used by that
application must be released before the main test routine can continue.

Why did exception "Port already in use error: " happen for Test1, then
the port was free for 3/4/6, and then the exception appeared again for
7, 8, 9?


Test1: fails; it tries to start JMX connector on port 50235 and check 
the conenction afterward

Test2: passes; does not use port 50235
Test3: passes; checks for not being able to connect to port 50235
Test4: fails; the same as Test1
Test5: passes; the same as Test2
Test6: fails; it tries to start RMI registry on port 50235 and fails
Test7: fails; the same as Test1
Test8: fails; the same as Test1
Test9: fails; the same as Test1
Test10: passes; the same as Test2
Test11: passes; the same as Test2
Test12: passes; the same as Test2
Test13: passes; the same as Test2

The port doesn't mysteriously become used and unused. It is still 
occupied by a different process. Some of the tests don't fail simply 
because they don't use the port.
Yes right, some tests passed because they did not expected JMX 
connection, not sure possible to distinguish between a port issue and no 
jmx server issue.


The fix looks OK.

Thanks,
Shanliang




A port is possibly unavailable after being closed, because it can be in
the state TIME_WAIT.

Your fix created a server socket but no client would connect to it, then
the port could be available immediately after close(), not need to enter
TIME_WAIT state, if so hopeful the fix could work.


SocketServer does not accept any incoming connection and as such the 
socket should not go to TIME_WAIT state when it is closed. The JPRT 
results would indicate that this is indeed the case.


-JB-



Shanliang




Your solution is to create a Server socket on a free port, then 
release
it when a test needs it. I suspect whether we will fall into same 
issue

here: the port would not be fully released when using it?


No. SocketServer.close() is called synchronously right before the port
is going to be used. This call unbinds the socket and returns. At the
moment of the return the port is free. I've run the tests locally and
via JPRT and they are all passing.

Thanks,

-JB-



Shanliang

Jaroslav Bachorik wrote:

Please, review this test fix.

Issue : https://bugs.openjdk.java.net/browse/JDK-8035395
Webrev: http://cr.openjdk.java.net/~jbachorik/8035395/webrev.00

Currently, the test is using two fixed ports to start JMX connector
and RMI registry when necessary. It can not deal with situations when
the ports are not available. The patch is adding the ability to 
obtain

ports from the ephemeral range and use them instead of the hardcoded
ones. It also tries to minimize the chance of another process 
stealing

the ports by holding the corresponding SocketServers open till right
before the port is actually needed.

Thanks,

-JB-

Re: RFR 8035395: sun/management/jmxremote/startstop/JMXStartStopTest.java fails intermittently: Port already in use

[shanliang]

Jaroslav Bachorik wrote:

Hi Shanliang,

On 20.2.2014 19:20, shanliang wrote:

Jaroslav,

The failed tests were:
1, 7, 8, 9

but the tests using this port (port2: 50235) were
1, 3, 4, 6, 7, 8, 9

and tests 2,4,6 were passed.

so I think that the problem might be that the port was not fully
released when a test was trying to use it, even the port was closed by
the previous test.


I don't think this is the case.

Firstly, test 1 fails. There are no previous tests possibly holding on 
to the required port. So the port must have been taken by some foreign 
process. The port number being from the ephemeral range doesn't help 
either, quite contrary.


Secondly, a port can not be partially bound - either a process binds 
to a port or not. Also, when a process exits all the bound ports must 
be released. Since we are waiting for the exit code of the launched 
test application before proceeding all the ports used by that 
application must be released before the main test routine can continue.
Why did exception "Port already in use error: " happen for Test1, then 
the port was free for 3/4/6, and then the exception appeared again for 
7, 8, 9?


A port is possibly unavailable after being closed, because it can be in 
the state TIME_WAIT.


Your fix created a server socket but no client would connect to it, then 
the port could be available immediately after close(), not need to enter 
TIME_WAIT state, if so hopeful the fix could work.


Shanliang




Your solution is to create a Server socket on a free port, then release
it when a test needs it. I suspect whether we will fall into same issue
here: the port would not be fully released when using it?


No. SocketServer.close() is called synchronously right before the port 
is going to be used. This call unbinds the socket and returns. At the 
moment of the return the port is free. I've run the tests locally and 
via JPRT and they are all passing.


Thanks,

-JB-



Shanliang

Jaroslav Bachorik wrote:

Please, review this test fix.

Issue : https://bugs.openjdk.java.net/browse/JDK-8035395
Webrev: http://cr.openjdk.java.net/~jbachorik/8035395/webrev.00

Currently, the test is using two fixed ports to start JMX connector
and RMI registry when necessary. It can not deal with situations when
the ports are not available. The patch is adding the ability to obtain
ports from the ephemeral range and use them instead of the hardcoded
ones. It also tries to minimize the chance of another process stealing
the ports by holding the corresponding SocketServers open till right
before the port is actually needed.

Thanks,

-JB-

Re: RFR 8035395: sun/management/jmxremote/startstop/JMXStartStopTest.java fails intermittently: Port already in use

[shanliang]

Jaroslav,

The failed tests were:
   1, 7, 8, 9

but the tests using this port (port2: 50235) were
   1, 3, 4, 6, 7, 8, 9

and tests 2,4,6 were passed.

so I think that the problem might be that the port was not fully 
released when a test was trying to use it, even the port was closed by 
the previous test.


Your solution is to create a Server socket on a free port, then release 
it when a test needs it. I suspect whether we will fall into same issue 
here: the port would not be fully released when using it?


Shanliang

Jaroslav Bachorik wrote:

Please, review this test fix.

Issue : https://bugs.openjdk.java.net/browse/JDK-8035395
Webrev: http://cr.openjdk.java.net/~jbachorik/8035395/webrev.00

Currently, the test is using two fixed ports to start JMX connector 
and RMI registry when necessary. It can not deal with situations when 
the ports are not available. The patch is adding the ability to obtain 
ports from the ephemeral range and use them instead of the hardcoded 
ones. It also tries to minimize the chance of another process stealing 
the ports by holding the corresponding SocketServers open till right 
before the port is actually needed.


Thanks,

-JB-

Codereview request: 8035195 demo/jvmti/mtrace/TraceJFrame.java can't connect to X11

[shanliang]

Hi,

The failure was from a bad DISPLAY setting, this must be not intended, 
so we re-throw the exception and tell to make sure that the DISPLAY is 
correct.


The fix is only to add a catch of InternalError at the beginning, the 
rest code is not modified but re-formated to remove some spaces.


webrev:
http://cr.openjdk.java.net/~sjiang/JDK-8035195/00 
<http://cr.openjdk.java.net/%7Esjiang/JDK-8035195/00>/


bug:
https://bugs.openjdk.java.net/browse/JDK-8035195


Thanks,
Shanliang

Re: RFR(S): 6952105 TEST_BUG: testcase failure, not very often, com/sun/jdi/SuspendThreadTest.java

[shanliang]

I am looking at the old file:
143 while (bkptCount < maxBkpts) {
144 prevBkptCount = bkptCount;

suppose the following execution sequence:
1)   when Line 143 was called by Thread1, we had bkptCount == maxBkpts - 1;

2) bkptCount++ was executed by thread2;

3) Line 144 was called by thread1,

in this case it was sure that the line
   152 failure("failure: test hung");
would be called.

It is good to add:
   synchronized (bkptSignal)
in the fix, but we need to put Line 143 and 144 into synchronization too.

To deal with a spurious wakeup, we might do like this:
   long stopTime = System.currentTimeMillis() + 5000;
   do {
   try {
   bkptSignal.wait(100);
   } catch (InterruptedException e){}
   } while(prevBkptCount == bkptCount && System.currentTimeMillis() 
< stopTime);


Shanliang

David Holmes wrote:

On 18/02/2014 11:03 PM, Staffan Larsen wrote:


On 18 feb 2014, at 13:09, David Holmes  wrote:


Hi Staffan,

If you get a spurious wakeup from wait():

151 try {
152 synchronized (bkptSignal) {
153 bkptSignal.wait(5000);
154 }
155 } catch (InterruptedException ee) {
156 }
157 if (prevBkptCount == bkptCount) {
158 failure("failure: test hung");

you could report failure. But that is far less likely than the 
current problem using sleep.


Right. Adding “continue;” inside the catch(InterruptedException) 
block should guard against that.


No, a spurious wakeup is not an interrupt - the wait() will simply 
return.


David


/Staffan



David

On 18/02/2014 8:19 PM, Staffan Larsen wrote:

Still looking for Reviewer for this change.

Thanks,
/Staffan

On 11 feb 2014, at 15:12, Staffan Larsen 
 wrote:


Updated the test to use proper synchronization and notification 
between threads. Should be more stable and much faster.


bug: https://bugs.openjdk.java.net/browse/JDK-6952105
webrev: http://cr.openjdk.java.net/~sla/6952105/webrev.00/

Thanks,
/Staffan

Re: Codereview request: 8007710 runtime/7158988/FieldMonitor.java fails with com.sun.jdi.VMDisconnectedException: Connection closed

[shanliang]

Staffan Larsen wrote:

This version looks good! Thanks for hanging in there.

The only improvement would be to count and verify the number of 
ModificationWatchpointEvent (there should be 10).

Good idea, here is:
http://cr.openjdk.java.net/~sjiang/JDK-8007710/05/

Thanks,
Shanliang


Thanks,
/Staffan

On 13 feb 2014, at 21:15, shanliang <mailto:shanliang.ji...@oracle.com>> wrote:



Hi,

Here is Version 4:
http://cr.openjdk.java.net/~sjiang/JDK-8007710/04/

1) remove the line
108   vm.resume()
2) call addClassWatch(vm) only when receiving VMStartEvent
3) make sure that the test receives ModificationWatchpointEvent
4) clean

Thanks,
Shanliang

shanliang wrote:

Staffan,

Very nice analysis!

The fix must be very simple, just remove the line
108   vm.resume
it is an error because here the test does not yet treat the events 
in eventSet.


the line
136   eventSet.resume();
is the right place to resume the threads after event treatment.

Here is the new webrev:
http://cr.openjdk.java.net/~sjiang/JDK-8007710/03/

Thanks,
Shanliang

Staffan Larsen wrote:

I think I understand what happens now.

The test code, simplified, looks like this (with the Thread.sleep() 
added that causes the test to fail):


  launchTarget();
  addClassWatch();
  vm.resume();
  Thread.sleep(1000);
  while(connected) {
  eventSet = eventQueue.remove()
  for(event : eventQueue) {
  if (event instanceof ClassPrepareEvent) {
  addFieldWatch();
  }
  }
  eventSet.resume();
  }

By default all events that happen will cause the debuggee to 
suspend (see EventRequest.setSuspendPolicy()). Thus when we get to 
addFieldWatch(), the vm should be suspended and we should be able 
to create the field watch without problem. But the VM isn’t 
suspended and that is why the test fail. 

Why isn’t the VM suspended? When we get to the “for(event : 
eventQueue)” the first time there are *two* events already in the 
queue: the VMStartEvent and a ClassPrepareEvent. At this point the 
VM is suspended and everything is good. We look at the first 
eventSet which only contains the VMStartEvent, we ignore the event, 
but we resume the VM. We then loop and look at the 
ClassPrepareEvent, but by now the VM is already running and has 
also terminated. Failure.


Thus, we need to handle the VMStartEvent. I suggest a modification 
to my previous code:


  launchTarget();
  while(connected) {
  eventSet = eventQueue.remove()
  for(event : eventQueue) {
  if (event instanceof VMStartEvent) {
  addClassWatch();
  }
  if (event instanceof ClassPrepareEvent) {
  addFieldWatch();
  }
  }
  eventSet.resume();
  }

This will cause us to have complete control over the state of the 
debuggee. The first event we see will be the VMStartEvent. The VM 
will be suspended. We can add a class watch here. Then we resume 
the VM. The second event we see will be the ClassPrepareEvent with 
the VM suspended. We can add the field watch. Then we resume the VM 
and wait for the field watch events.


Thanks,
/Staffan

On 13 feb 2014, at 11:36, shanliang <mailto:shanliang.ji...@oracle.com>> wrote:



Staffan Larsen wrote:

On 13 feb 2014, at 10:17, Jaroslav Bachorik  
wrote:

  

Hi Staffan,

On 12.2.2014 18:27, Staffan Larsen wrote:


I’m still not happy with this fix since I think the extra output stream 
synchronization logic is not needed - the debuggee should be suspended at all 
the interesting points. The fix I proposed is cleaner and (as far as I can 
tell) also fixes the problem. The only thing is that I can’t quite explain what 
goes wrong without the fix… I’d really like to understand that. I’ll try to dig 
deeper and see if I can understand exactly what happens.
  

Yes, bringing the VM to a stable state before calling other JDI functions helps 
to stabilize the test even without the additional synchronization via 
stdout/stdin.

I just wonder whether this check should not be done inside 
com.sun.jdi.connect.LaunchingConnector#launch() implementation. Does it even 
make sense to hand off an unstable VM?


Good question, but hard to change now - all implementations depend on the 
current functionality. The VMStartEvent also gives you a reference to the main 
thread.
  
The test failed when it received ClassPrepareEvent and did 
addFieldWatch, that meant the test must receive already 
VMStartEvent, because VMStartEvent must be the first event, if it 
was true then the vm must be already stable when failing.


Except that the test received ClassPrepareEvent before 
VMStartEvent then it was doing addFieldWatch with a possibly 
unstable VM. in this case we might have a serious bug in 
VirtualMachine implementation, and if this is true the fix 
proposed to check "start" may make miss ClassPrepareEvent, then 
the test would test nothing.


Shanliang

/S

  

-JB-



/Staffan

On 12 feb 2014, at 18:04, shanliang  wr

Re: Codereview request: 8007710 runtime/7158988/FieldMonitor.java fails with com.sun.jdi.VMDisconnectedException: Connection closed

[shanliang]

Hi,

Here is Version 4:
   http://cr.openjdk.java.net/~sjiang/JDK-8007710/04/

1) remove the line
   108   vm.resume()
2) call addClassWatch(vm) only when receiving VMStartEvent
3) make sure that the test receives ModificationWatchpointEvent
4) clean

Thanks,
Shanliang

shanliang wrote:

Staffan,

Very nice analysis!

The fix must be very simple, just remove the line
108   vm.resume
it is an error because here the test does not yet treat the events in 
eventSet.


the line
136   eventSet.resume();
is the right place to resume the threads after event treatment.

Here is the new webrev:
http://cr.openjdk.java.net/~sjiang/JDK-8007710/03/

Thanks,
Shanliang

Staffan Larsen wrote:

I think I understand what happens now.

The test code, simplified, looks like this (with the Thread.sleep() 
added that causes the test to fail):


  launchTarget();
  addClassWatch();
  vm.resume();
  Thread.sleep(1000);
  while(connected) {
  eventSet = eventQueue.remove()
  for(event : eventQueue) {
  if (event instanceof ClassPrepareEvent) {
  addFieldWatch();
  }
  }
  eventSet.resume();
  }

By default all events that happen will cause the debuggee to suspend 
(see EventRequest.setSuspendPolicy()). Thus when we get to 
addFieldWatch(), the vm should be suspended and we should be able to 
create the field watch without problem. But the VM isn’t suspended 
and that is why the test fail. 

Why isn’t the VM suspended? When we get to the “for(event : 
eventQueue)” the first time there are *two* events already in the 
queue: the VMStartEvent and a ClassPrepareEvent. At this point the VM 
is suspended and everything is good. We look at the first eventSet 
which only contains the VMStartEvent, we ignore the event, but we 
resume the VM. We then loop and look at the ClassPrepareEvent, but by 
now the VM is already running and has also terminated. Failure.


Thus, we need to handle the VMStartEvent. I suggest a modification to 
my previous code:


  launchTarget();
  while(connected) {
  eventSet = eventQueue.remove()
  for(event : eventQueue) {
  if (event instanceof VMStartEvent) {
  addClassWatch();
  }
  if (event instanceof ClassPrepareEvent) {
  addFieldWatch();
  }
  }
  eventSet.resume();
  }

This will cause us to have complete control over the state of the 
debuggee. The first event we see will be the VMStartEvent. The VM 
will be suspended. We can add a class watch here. Then we resume the 
VM. The second event we see will be the ClassPrepareEvent with the VM 
suspended. We can add the field watch. Then we resume the VM and wait 
for the field watch events.


Thanks,
/Staffan

On 13 feb 2014, at 11:36, shanliang <mailto:shanliang.ji...@oracle.com>> wrote:



Staffan Larsen wrote:

On 13 feb 2014, at 10:17, Jaroslav Bachorik  
wrote:

  

Hi Staffan,

On 12.2.2014 18:27, Staffan Larsen wrote:


I’m still not happy with this fix since I think the extra output stream 
synchronization logic is not needed - the debuggee should be suspended at all 
the interesting points. The fix I proposed is cleaner and (as far as I can 
tell) also fixes the problem. The only thing is that I can’t quite explain what 
goes wrong without the fix… I’d really like to understand that. I’ll try to dig 
deeper and see if I can understand exactly what happens.
  

Yes, bringing the VM to a stable state before calling other JDI functions helps 
to stabilize the test even without the additional synchronization via 
stdout/stdin.

I just wonder whether this check should not be done inside 
com.sun.jdi.connect.LaunchingConnector#launch() implementation. Does it even 
make sense to hand off an unstable VM?


Good question, but hard to change now - all implementations depend on the 
current functionality. The VMStartEvent also gives you a reference to the main 
thread.
  
The test failed when it received ClassPrepareEvent and did 
addFieldWatch, that meant the test must receive already 
VMStartEvent, because VMStartEvent must be the first event, if it 
was true then the vm must be already stable when failing.


Except that the test received ClassPrepareEvent before VMStartEvent 
then it was doing addFieldWatch with a possibly unstable VM. in this 
case we might have a serious bug in VirtualMachine implementation, 
and if this is true the fix proposed to check "start" may make miss 
ClassPrepareEvent, then the test would test nothing.


Shanliang

/S

  

-JB-



/Staffan

On 12 feb 2014, at 18:04, shanliang  wrote:

  

Staffan Larsen wrote:


I think what you need to do is wait for the VMStartEvent before you add 
requests to the VM. Note this paragraph from the VirtualMachine doc:

 Note that a target VM launched by a launching connector is not
 guaranteed to be stable until after the VMStartEvent has been
 received.

  

I may miss something here, I believe VMStartEvent must be th

Re: Codereview request: 8007710 runtime/7158988/FieldMonitor.java fails with com.sun.jdi.VMDisconnectedException: Connection closed

[shanliang]

Staffan,

Very nice analysis!

The fix must be very simple, just remove the line
   108   vm.resume
it is an error because here the test does not yet treat the events in 
eventSet.


the line
   136   eventSet.resume();
is the right place to resume the threads after event treatment.

Here is the new webrev:
http://cr.openjdk.java.net/~sjiang/JDK-8007710/03/

Thanks,
Shanliang

Staffan Larsen wrote:

I think I understand what happens now.

The test code, simplified, looks like this (with the Thread.sleep() 
added that causes the test to fail):


  launchTarget();
  addClassWatch();
  vm.resume();
  Thread.sleep(1000);
  while(connected) {
  eventSet = eventQueue.remove()
  for(event : eventQueue) {
  if (event instanceof ClassPrepareEvent) {
  addFieldWatch();
  }
  }
  eventSet.resume();
  }

By default all events that happen will cause the debuggee to suspend 
(see EventRequest.setSuspendPolicy()). Thus when we get to 
addFieldWatch(), the vm should be suspended and we should be able to 
create the field watch without problem. But the VM isn’t suspended and 
that is why the test fail. 

Why isn’t the VM suspended? When we get to the “for(event : 
eventQueue)” the first time there are *two* events already in the 
queue: the VMStartEvent and a ClassPrepareEvent. At this point the VM 
is suspended and everything is good. We look at the first eventSet 
which only contains the VMStartEvent, we ignore the event, but we 
resume the VM. We then loop and look at the ClassPrepareEvent, but by 
now the VM is already running and has also terminated. Failure.


Thus, we need to handle the VMStartEvent. I suggest a modification to 
my previous code:


  launchTarget();
  while(connected) {
  eventSet = eventQueue.remove()
  for(event : eventQueue) {
  if (event instanceof VMStartEvent) {
  addClassWatch();
  }
  if (event instanceof ClassPrepareEvent) {
  addFieldWatch();
  }
  }
  eventSet.resume();
  }

This will cause us to have complete control over the state of the 
debuggee. The first event we see will be the VMStartEvent. The VM will 
be suspended. We can add a class watch here. Then we resume the VM. 
The second event we see will be the ClassPrepareEvent with the VM 
suspended. We can add the field watch. Then we resume the VM and wait 
for the field watch events.


Thanks,
/Staffan

On 13 feb 2014, at 11:36, shanliang <mailto:shanliang.ji...@oracle.com>> wrote:



Staffan Larsen wrote:

On 13 feb 2014, at 10:17, Jaroslav Bachorik  
wrote:

  

Hi Staffan,

On 12.2.2014 18:27, Staffan Larsen wrote:


I’m still not happy with this fix since I think the extra output stream 
synchronization logic is not needed - the debuggee should be suspended at all 
the interesting points. The fix I proposed is cleaner and (as far as I can 
tell) also fixes the problem. The only thing is that I can’t quite explain what 
goes wrong without the fix… I’d really like to understand that. I’ll try to dig 
deeper and see if I can understand exactly what happens.
  

Yes, bringing the VM to a stable state before calling other JDI functions helps 
to stabilize the test even without the additional synchronization via 
stdout/stdin.

I just wonder whether this check should not be done inside 
com.sun.jdi.connect.LaunchingConnector#launch() implementation. Does it even 
make sense to hand off an unstable VM?


Good question, but hard to change now - all implementations depend on the 
current functionality. The VMStartEvent also gives you a reference to the main 
thread.
  
The test failed when it received ClassPrepareEvent and did 
addFieldWatch, that meant the test must receive already VMStartEvent, 
because VMStartEvent must be the first event, if it was true then the 
vm must be already stable when failing.


Except that the test received ClassPrepareEvent before VMStartEvent 
then it was doing addFieldWatch with a possibly unstable VM. in this 
case we might have a serious bug in VirtualMachine implementation, 
and if this is true the fix proposed to check "start" may make miss 
ClassPrepareEvent, then the test would test nothing.


Shanliang

/S

  

-JB-



/Staffan

On 12 feb 2014, at 18:04, shanliang  wrote:

  

Staffan Larsen wrote:


I think what you need to do is wait for the VMStartEvent before you add 
requests to the VM. Note this paragraph from the VirtualMachine doc:

 Note that a target VM launched by a launching connector is not
 guaranteed to be stable until after the VMStartEvent has been
 received.

  

I may miss something here, I believe VMStartEvent must be the first event, when 
the test got ClassPrepareEvent, it must already received VMStartEvent.


I think adding code that looks something like this will make the test stable:

VirtualMachine vm = launchTarget(CLASS_NAME);
EventQueue eventQueue = vm.eventQueue();

bool

Re: Codereview request: 8007710 runtime/7158988/FieldMonitor.java fails with com.sun.jdi.VMDisconnectedException: Connection closed

[shanliang]

Staffan Larsen wrote:

On 13 feb 2014, at 10:17, Jaroslav Bachorik  
wrote:

  

Hi Staffan,

On 12.2.2014 18:27, Staffan Larsen wrote:


I’m still not happy with this fix since I think the extra output stream 
synchronization logic is not needed - the debuggee should be suspended at all 
the interesting points. The fix I proposed is cleaner and (as far as I can 
tell) also fixes the problem. The only thing is that I can’t quite explain what 
goes wrong without the fix… I’d really like to understand that. I’ll try to dig 
deeper and see if I can understand exactly what happens.
  

Yes, bringing the VM to a stable state before calling other JDI functions helps 
to stabilize the test even without the additional synchronization via 
stdout/stdin.

I just wonder whether this check should not be done inside 
com.sun.jdi.connect.LaunchingConnector#launch() implementation. Does it even 
make sense to hand off an unstable VM?



Good question, but hard to change now - all implementations depend on the 
current functionality. The VMStartEvent also gives you a reference to the main 
thread.
  
The test failed when it received ClassPrepareEvent and did 
addFieldWatch, that meant the test must receive already VMStartEvent, 
because VMStartEvent must be the first event, if it was true then the vm 
must be already stable when failing.


Except that the test received ClassPrepareEvent before VMStartEvent then 
it was doing addFieldWatch with a possibly unstable VM. in this case we 
might have a serious bug in VirtualMachine implementation, and if this 
is true the fix proposed to check "start" may make miss 
ClassPrepareEvent, then the test would test nothing.


Shanliang

/S

  

-JB-



/Staffan

On 12 feb 2014, at 18:04, shanliang  wrote:

  

Staffan Larsen wrote:


I think what you need to do is wait for the VMStartEvent before you add 
requests to the VM. Note this paragraph from the VirtualMachine doc:

 Note that a target VM launched by a launching connector is not
 guaranteed to be stable until after the VMStartEvent has been
 received.

  

I may miss something here, I believe VMStartEvent must be the first event, when 
the test got ClassPrepareEvent, it must already received VMStartEvent.


I think adding code that looks something like this will make the test stable:

VirtualMachine vm = launchTarget(CLASS_NAME);
EventQueue eventQueue = vm.eventQueue();

boolean started = false;
while(!started) {
  EventSet eventSet = eventQueue.remove();
  for (Event event : eventSet) {
if (event instanceof VMStartEvent) {
  started = true;
}
if (event instanceof VMDeathEvent
|| event instanceof VMDisconnectEvent) {
  throw new Error("VM died before it started...:"+event);
}
  }
}

System.out.println("Vm launched");

  

The code you proposed could improve the test, it made sure that 
TestPostFieldModification was started, but I am afraid that it did not address 
the issue causing the failure, the issue I believe was that 
TestPostFieldModification exited before or during FieldMonitor called 
addFieldWatch(), that was why addFieldWatch() received VMDisconnectedException. 
When the test was treating ClassPrepareEvent, even if VMDeathEvent or 
VMDisconnectEvent arrived, it must be still waiting in the eventQueue because 
it arrived after ClassPrepareEvent.

My fix was to not allow TestPostFieldModification to exit before 
addFieldWatch() was done.


There is also no reason to call addFieldWatch() before the ClassPrepareEvent 
has been received. The call to vm..classesByName() will just return an empty 
list anyway.

  

I do not know why the test called addFieldWatch before ClassPrepareEvent had 
been received, but yes the returned list was empty, so agree to remove it.


While you are in there you can also remove the unused StringBuffer near the top 
of main().

  

Yes it was already removed in version 01

Here is the new webrev:
http://cr.openjdk.java.net/~sjiang/JDK-8007710/02/

Thanks,
Shanliang


Thanks,
/Staffan

On 11 feb 2014, at 18:30, shanliang  wrote:


  

Here is the new fix in which FieldMonitor will write to 
TestPostFieldModification, to inform the latter to quit, as suggested bu 
Jaroslav
  http://cr.openjdk.java.net/~sjiang/JDK-8007710/01/

Thanks,
Shanliang

shanliang wrote:



shanliang wrote:

  

Jaroslav Bachorik wrote:



On 11.2.2014 16:31, shanliang wrote:

  

Staffan Larsen wrote:



Hi Shanliang,

I can’t quite see how the test can fail in this way. When the
ClassPrepareEvent happens, the debuggee will be suspended. So when
addFieldWatch() is called, the debuggee should not have moved.

  

I am not expert of jdi so I may miss something here. I checked the
failure trace and s

Re: Codereview request: 8007710 runtime/7158988/FieldMonitor.java fails with com.sun.jdi.VMDisconnectedException: Connection closed

[shanliang]

serguei.spit...@oracle.com wrote:

The fix looks good.
But could you change "impossible" at line 45 to something more 
adequate, i.e. "caught exception"? :


  41 System.out.println("---TestPostFieldModification-run waiting to exit 
...");
  42 try {
  43 System.in.read();
  44 } catch (Exception e) {
  45 System.out.println("---TestPostFieldModification-run impossible? 
"+e);
  46 e.printStackTrace();
  47 }


Done.
Thanks for reviewing.
Shanliang


Thanks,
Serguei


On 2/11/14 9:30 AM, shanliang wrote:
Here is the new fix in which FieldMonitor will write to 
TestPostFieldModification, to inform the latter to quit, as suggested 
bu Jaroslav

   http://cr.openjdk.java.net/~sjiang/JDK-8007710/01/

Thanks,
Shanliang

shanliang wrote:

shanliang wrote:

Jaroslav Bachorik wrote:

On 11.2.2014 16:31, shanliang wrote:

Staffan Larsen wrote:

Hi Shanliang,

I can’t quite see how the test can fail in this way. When the
ClassPrepareEvent happens, the debuggee will be suspended. So when
addFieldWatch() is called, the debuggee should not have moved.

I am not expert of jdi so I may miss something here. I checked the
failure trace and saw the report exception happen when FieldMonitor
received ClassPrepareEvent and was doing addFieldWatch. 
FieldMonitor did

call "vm.resume()" before treating events.


AFAICS, calling vm.resume() results in an almost immediate 
debuggee death. The gc() invoking thread "d" is flagged as a 
deamon and as such doesn't prevent the process from exiting. The 
other thread is not a daemon but will finish in only few cycles.
I looked at the class com.sun.jdi.VirtualMachine, here is the 
Javadoc of the method "resume":

   /**
* Continues the execution of the application running in this
* virtual machine. All threads are resumed as documented in
* {@link ThreadReference#resume}.
*
* @throws VMCannotBeModifiedException if the VirtualMachine is 
read-only - see {@link VirtualMachine#canBeModified()}.

*
* @see #suspend
*/
   void resume();
My understanding is that the debuggee resumes to work after this 
call, instead to die?
In fact the problem is here, the vm (TestPostFieldModification) 
should not die before FieldMonitor finishes addFieldWatch.


Shanliang




I reproduced the bug by add sleep(1000) after vm.resume() but before
calling eventQueue.remove();


It looks like some kind of synchronization between the debugger 
and the debuggee is necessary. But I wonder if you should better 
use the process.getOuptuptStream() to write and flush a message 
for the debugee indicating that it can exit. And in the debugee 
you would just do System.in.read() as the last statement in the 
main() method. Seems more robust than involving files.
It could work, but creating a file in the testing directory should 
have no issue, but yes maybe less performance.


Thanks,
Shanliang


Cheers,

-JB-



Thanks,
Shanliang

One problem I do see with the test is that it does not wait for a
VMStartEvent before setting up requests. I’m not sure if that could
cause the failure in the bug report, though.

/Staffan

On 11 feb 2014, at 15:13, shanliang  
wrote:



Hi ,

The problem could be that FieldMonitor did not have enough time to
"addFieldWatch" but the vm to monitor 
(TestPostFieldModification) was

already ended.

So we should make sure that TestPostFieldModification exits after
FieldMonitor has done necessary. The solution proposed here is 
that

FieldMonitor creates a file after adding field watching, and
TestPostFieldModification quits only after finding the file.

web:
http://icncweb.fr.oracle.com/~shjiang/webrev/8007710/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-8007710

Thanks,
Shanliang

Re: Codereview request: 8007710 runtime/7158988/FieldMonitor.java fails with com.sun.jdi.VMDisconnectedException: Connection closed

[shanliang]

Staffan Larsen wrote:

I think what you need to do is wait for the VMStartEvent before you add 
requests to the VM. Note this paragraph from the VirtualMachine doc:

 Note that a target VM launched by a launching connector is not
 guaranteed to be stable until after the VMStartEvent has been
 received.
  
I may miss something here, I believe VMStartEvent must be the first 
event, when the test got ClassPrepareEvent, it must already received 
VMStartEvent.

I think adding code that looks something like this will make the test stable:

VirtualMachine vm = launchTarget(CLASS_NAME);
EventQueue eventQueue = vm.eventQueue();

boolean started = false;
while(!started) {
  EventSet eventSet = eventQueue.remove();
  for (Event event : eventSet) {
if (event instanceof VMStartEvent) {
  started = true;
}
if (event instanceof VMDeathEvent
|| event instanceof VMDisconnectEvent) {
  throw new Error("VM died before it started...:"+event);
}
  }
}

System.out.println("Vm launched");
  
The code you proposed could improve the test, it made sure that 
TestPostFieldModification was started, but I am afraid that it did not 
address the issue causing the failure, the issue I believe was that 
TestPostFieldModification exited before or during FieldMonitor called 
addFieldWatch(), that was why addFieldWatch() received 
VMDisconnectedException. When the test was treating ClassPrepareEvent, 
even if VMDeathEvent or VMDisconnectEvent arrived, it must be still 
waiting in the eventQueue because it arrived after ClassPrepareEvent.


My fix was to not allow TestPostFieldModification to exit before 
addFieldWatch() was done. 
  
There is also no reason to call addFieldWatch() before the ClassPrepareEvent has been received. The call to vm..classesByName() will just return an empty list anyway.
  
I do not know why the test called addFieldWatch before ClassPrepareEvent 
had been received, but yes the returned list was empty, so agree to 
remove it.

While you are in there you can also remove the unused StringBuffer near the top 
of main().
  

Yes it was already removed in version 01

Here is the new webrev:
http://cr.openjdk.java.net/~sjiang/JDK-8007710/02/

Thanks,
Shanliang
 
Thanks,

/Staffan

On 11 feb 2014, at 18:30, shanliang  wrote:

  

Here is the new fix in which FieldMonitor will write to 
TestPostFieldModification, to inform the latter to quit, as suggested bu 
Jaroslav
  http://cr.openjdk.java.net/~sjiang/JDK-8007710/01/

Thanks,
Shanliang

shanliang wrote:


shanliang wrote:
  

Jaroslav Bachorik wrote:


On 11.2.2014 16:31, shanliang wrote:
  

Staffan Larsen wrote:
    

Hi Shanliang,

I can’t quite see how the test can fail in this way. When the
ClassPrepareEvent happens, the debuggee will be suspended. So when
addFieldWatch() is called, the debuggee should not have moved.
  

I am not expert of jdi so I may miss something here. I checked the
failure trace and saw the report exception happen when FieldMonitor
received ClassPrepareEvent and was doing addFieldWatch. FieldMonitor did
call "vm.resume()" before treating events.


AFAICS, calling vm.resume() results in an almost immediate debuggee death. The gc() 
invoking thread "d" is flagged as a deamon and as such doesn't prevent the 
process from exiting. The other thread is not a daemon but will finish in only few cycles.
  

I looked at the class com.sun.jdi.VirtualMachine, here is the Javadoc of the method 
"resume":
  /**
   * Continues the execution of the application running in this
   * virtual machine. All threads are resumed as documented in
   * {@link ThreadReference#resume}.
   *
   * @throws VMCannotBeModifiedException if the VirtualMachine is read-only - 
see {@link VirtualMachine#canBeModified()}.
   *
   * @see #suspend
   */
  void resume();
My understanding is that the debuggee resumes to work after this call, instead 
to die?


In fact the problem is here, the vm (TestPostFieldModification) should not die 
before FieldMonitor finishes addFieldWatch.

Shanliang
  

I reproduced the bug by add sleep(1000) after vm.resume() but before
calling eventQueue.remove();


It looks like some kind of synchronization between the debugger and the 
debuggee is necessary. But I wonder if you should better use the 
process.getOuptuptStream() to write and flush a message for the debugee 
indicating that it can exit. And in the debugee you would just do 
System.in.read() as the last statement in the main() method. Seems more robust 
than involving files.
  

It could work, but creating a file in the testing directory should have no 
issue, but yes maybe less performance.

Thanks,
Shanliang


Cheers,

-JB-

  

Thanks,
Shanliang


One problem I do see with the test is that it does not wait for a
V

Re: Codereview request: 8007710 runtime/7158988/FieldMonitor.java fails with com.sun.jdi.VMDisconnectedException: Connection closed

[shanliang]

Here is the new fix in which FieldMonitor will write to 
TestPostFieldModification, to inform the latter to quit, as suggested bu 
Jaroslav

   http://cr.openjdk.java.net/~sjiang/JDK-8007710/01/

Thanks,
Shanliang

shanliang wrote:

shanliang wrote:

Jaroslav Bachorik wrote:

On 11.2.2014 16:31, shanliang wrote:

Staffan Larsen wrote:

Hi Shanliang,

I can’t quite see how the test can fail in this way. When the
ClassPrepareEvent happens, the debuggee will be suspended. So when
addFieldWatch() is called, the debuggee should not have moved.

I am not expert of jdi so I may miss something here. I checked the
failure trace and saw the report exception happen when FieldMonitor
received ClassPrepareEvent and was doing addFieldWatch. 
FieldMonitor did

call "vm.resume()" before treating events.


AFAICS, calling vm.resume() results in an almost immediate debuggee 
death. The gc() invoking thread "d" is flagged as a deamon and as 
such doesn't prevent the process from exiting. The other thread is 
not a daemon but will finish in only few cycles.
I looked at the class com.sun.jdi.VirtualMachine, here is the Javadoc 
of the method "resume":

   /**
* Continues the execution of the application running in this
* virtual machine. All threads are resumed as documented in
* {@link ThreadReference#resume}.
*
* @throws VMCannotBeModifiedException if the VirtualMachine is 
read-only - see {@link VirtualMachine#canBeModified()}.

*
* @see #suspend
*/
   void resume();
My understanding is that the debuggee resumes to work after this 
call, instead to die?
In fact the problem is here, the vm (TestPostFieldModification) should 
not die before FieldMonitor finishes addFieldWatch.


Shanliang




I reproduced the bug by add sleep(1000) after vm.resume() but before
calling eventQueue.remove();


It looks like some kind of synchronization between the debugger and 
the debuggee is necessary. But I wonder if you should better use the 
process.getOuptuptStream() to write and flush a message for the 
debugee indicating that it can exit. And in the debugee you would 
just do System.in.read() as the last statement in the main() method. 
Seems more robust than involving files.
It could work, but creating a file in the testing directory should 
have no issue, but yes maybe less performance.


Thanks,
Shanliang


Cheers,

-JB-



Thanks,
Shanliang

One problem I do see with the test is that it does not wait for a
VMStartEvent before setting up requests. I’m not sure if that could
cause the failure in the bug report, though.

/Staffan

On 11 feb 2014, at 15:13, shanliang  
wrote:



Hi ,

The problem could be that FieldMonitor did not have enough time to
"addFieldWatch" but the vm to monitor (TestPostFieldModification) 
was

already ended.

So we should make sure that TestPostFieldModification exits after
FieldMonitor has done necessary. The solution proposed here is that
FieldMonitor creates a file after adding field watching, and
TestPostFieldModification quits only after finding the file.

web:
http://icncweb.fr.oracle.com/~shjiang/webrev/8007710/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-8007710

Thanks,
Shanliang

Re: Codereview request: 8007710 runtime/7158988/FieldMonitor.java fails with com.sun.jdi.VMDisconnectedException: Connection closed

[shanliang]

shanliang wrote:

Jaroslav Bachorik wrote:

On 11.2.2014 16:31, shanliang wrote:

Staffan Larsen wrote:

Hi Shanliang,

I can’t quite see how the test can fail in this way. When the
ClassPrepareEvent happens, the debuggee will be suspended. So when
addFieldWatch() is called, the debuggee should not have moved.

I am not expert of jdi so I may miss something here. I checked the
failure trace and saw the report exception happen when FieldMonitor
received ClassPrepareEvent and was doing addFieldWatch. FieldMonitor 
did

call "vm.resume()" before treating events.


AFAICS, calling vm.resume() results in an almost immediate debuggee 
death. The gc() invoking thread "d" is flagged as a deamon and as 
such doesn't prevent the process from exiting. The other thread is 
not a daemon but will finish in only few cycles.
I looked at the class com.sun.jdi.VirtualMachine, here is the Javadoc 
of the method "resume":

   /**
* Continues the execution of the application running in this
* virtual machine. All threads are resumed as documented in
* {@link ThreadReference#resume}.
*
* @throws VMCannotBeModifiedException if the VirtualMachine is 
read-only - see {@link VirtualMachine#canBeModified()}.

*
* @see #suspend
*/
   void resume();
My understanding is that the debuggee resumes to work after this call, 
instead to die?
In fact the problem is here, the vm (TestPostFieldModification) should 
not die before FieldMonitor finishes addFieldWatch.


Shanliang




I reproduced the bug by add sleep(1000) after vm.resume() but before
calling eventQueue.remove();


It looks like some kind of synchronization between the debugger and 
the debuggee is necessary. But I wonder if you should better use the 
process.getOuptuptStream() to write and flush a message for the 
debugee indicating that it can exit. And in the debugee you would 
just do System.in.read() as the last statement in the main() method. 
Seems more robust than involving files.
It could work, but creating a file in the testing directory should 
have no issue, but yes maybe less performance.


Thanks,
Shanliang


Cheers,

-JB-



Thanks,
Shanliang

One problem I do see with the test is that it does not wait for a
VMStartEvent before setting up requests. I’m not sure if that could
cause the failure in the bug report, though.

/Staffan

On 11 feb 2014, at 15:13, shanliang  
wrote:



Hi ,

The problem could be that FieldMonitor did not have enough time to
"addFieldWatch" but the vm to monitor (TestPostFieldModification) was
already ended.

So we should make sure that TestPostFieldModification exits after
FieldMonitor has done necessary. The solution proposed here is that
FieldMonitor creates a file after adding field watching, and
TestPostFieldModification quits only after finding the file.

web:
http://icncweb.fr.oracle.com/~shjiang/webrev/8007710/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-8007710

Thanks,
Shanliang

Re: Codereview request: 8007710 runtime/7158988/FieldMonitor.java fails with com.sun.jdi.VMDisconnectedException: Connection closed

[shanliang]

Jaroslav Bachorik wrote:

On 11.2.2014 16:31, shanliang wrote:

Staffan Larsen wrote:

Hi Shanliang,

I can’t quite see how the test can fail in this way. When the
ClassPrepareEvent happens, the debuggee will be suspended. So when
addFieldWatch() is called, the debuggee should not have moved.

I am not expert of jdi so I may miss something here. I checked the
failure trace and saw the report exception happen when FieldMonitor
received ClassPrepareEvent and was doing addFieldWatch. FieldMonitor did
call "vm.resume()" before treating events.


AFAICS, calling vm.resume() results in an almost immediate debuggee 
death. The gc() invoking thread "d" is flagged as a deamon and as such 
doesn't prevent the process from exiting. The other thread is not a 
daemon but will finish in only few cycles.
I looked at the class com.sun.jdi.VirtualMachine, here is the Javadoc of 
the method "resume":

   /**
* Continues the execution of the application running in this
* virtual machine. All threads are resumed as documented in
* {@link ThreadReference#resume}.
*
* @throws VMCannotBeModifiedException if the VirtualMachine is 
read-only - see {@link VirtualMachine#canBeModified()}.

*
* @see #suspend
*/
   void resume();
My understanding is that the debuggee resumes to work after this call, 
instead to die?




I reproduced the bug by add sleep(1000) after vm.resume() but before
calling eventQueue.remove();


It looks like some kind of synchronization between the debugger and 
the debuggee is necessary. But I wonder if you should better use the 
process.getOuptuptStream() to write and flush a message for the 
debugee indicating that it can exit. And in the debugee you would just 
do System.in.read() as the last statement in the main() method. Seems 
more robust than involving files.
It could work, but creating a file in the testing directory should have 
no issue, but yes maybe less performance.


Thanks,
Shanliang


Cheers,

-JB-



Thanks,
Shanliang

One problem I do see with the test is that it does not wait for a
VMStartEvent before setting up requests. I’m not sure if that could
cause the failure in the bug report, though.

/Staffan

On 11 feb 2014, at 15:13, shanliang  wrote:


Hi ,

The problem could be that FieldMonitor did not have enough time to
"addFieldWatch" but the vm to monitor (TestPostFieldModification) was
already ended.

So we should make sure that TestPostFieldModification exits after
FieldMonitor has done necessary. The solution proposed here is that
FieldMonitor creates a file after adding field watching, and
TestPostFieldModification quits only after finding the file.

web:
http://icncweb.fr.oracle.com/~shjiang/webrev/8007710/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-8007710

Thanks,
Shanliang

Re: Codereview request: 8007710 runtime/7158988/FieldMonitor.java fails with com.sun.jdi.VMDisconnectedException: Connection closed

[shanliang]

Staffan Larsen wrote:

Hi Shanliang,

I can’t quite see how the test can fail in this way. When the ClassPrepareEvent 
happens, the debuggee will be suspended. So when addFieldWatch() is called, the 
debuggee should not have moved.
  
I am not expert of jdi so I may miss something here. I checked the 
failure trace and saw the report exception happen when FieldMonitor 
received ClassPrepareEvent and was doing addFieldWatch. FieldMonitor did 
call "vm.resume()" before treating events.


I reproduced the bug by add sleep(1000) after vm.resume() but before 
calling eventQueue.remove();


Thanks,
Shanliang

One problem I do see with the test is that it does not wait for a VMStartEvent 
before setting up requests. I’m not sure if that could cause the failure in the 
bug report, though.

/Staffan

On 11 feb 2014, at 15:13, shanliang  wrote:

  

Hi ,

The problem could be that FieldMonitor did not have enough time to 
"addFieldWatch" but the vm to monitor (TestPostFieldModification) was already 
ended.

So we should make sure that TestPostFieldModification exits after FieldMonitor 
has done necessary. The solution proposed here is that FieldMonitor creates a 
file after adding field watching, and TestPostFieldModification quits only 
after finding the file.

web:
http://icncweb.fr.oracle.com/~shjiang/webrev/8007710/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-8007710

Thanks,
Shanliang



  

Re: Codereview request: 8007710 runtime/7158988/FieldMonitor.java fails with com.sun.jdi.VMDisconnectedException: Connection closed

[shanliang]

public web:
http://cr.openjdk.java.net/~sjiang/JDK-8007710/00/

Shanliang

shanliang wrote:

Hi ,

The problem could be that FieldMonitor did not have enough time to 
"addFieldWatch" but the vm to monitor (TestPostFieldModification) was 
already ended.


So we should make sure that TestPostFieldModification exits after 
FieldMonitor has done necessary. The solution proposed here is that 
FieldMonitor creates a file after adding field watching, and 
TestPostFieldModification quits only after finding the file.


web:
http://icncweb.fr.oracle.com/~shjiang/webrev/8007710/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-8007710

Thanks,
Shanliang

Codereview request: 8007710 runtime/7158988/FieldMonitor.java fails with com.sun.jdi.VMDisconnectedException: Connection closed

[shanliang]

Hi ,

The problem could be that FieldMonitor did not have enough time to 
"addFieldWatch" but the vm to monitor (TestPostFieldModification) was 
already ended.


So we should make sure that TestPostFieldModification exits after 
FieldMonitor has done necessary. The solution proposed here is that 
FieldMonitor creates a file after adding field watching, and 
TestPostFieldModification quits only after finding the file.


web:
http://icncweb.fr.oracle.com/~shjiang/webrev/8007710/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-8007710

Thanks,
Shanliang

Re: RFR 6791551: ExclusiveBind.java has a race condition

[shanliang]

Jaroslav Bachorik wrote:

On 4.2.2014 09:54, shanliang wrote:

Jaroslav,

Your fix should work in most case, but is it better and more reliable to
wait a VM event as suggested in the bug? even your timeout is adapted to
the test time factory, but the solution still depends to a fixed timeout
and a  fixed line out.


Well, if I get the test logic correctly it is supposed to test that 
the agent blocks the port even when no client has connected yet. 
Connecting to the agent and waiting for the event would change the 
thing the test checks, actually.
You are right that the test should not attach a VM before launching the 
second debuggee. Let's hope that 5000 * Utils.TIMEOUT_FACTOR works for 
all testing machines.

Looks OK.

Thanks,
Shanliang


-JB-



Shanliang

Jaroslav Bachorik wrote:

Please, review the following test fix:

Issue : https://bugs.openjdk.java.net/browse/JDK-6791551
Webrev: http://cr.openjdk.java.net/~jbachorik/6791551/webrev.00

The fix prevents the situation when the first debuggee has not managed
to finish its intialization while the second one is started up thus
making the port available for the second debuggee and failing the test.

The patch is using the library methods to configure and launch the
debuggee and the test waits for the well known string to appear in the
first debuggee output before attempting to launch the second debuggee.

Thanks,

-JB-

Re: RFR 6791551: ExclusiveBind.java has a race condition

[shanliang]

Jaroslav,

Your fix should work in most case, but is it better and more reliable to 
wait a VM event as suggested in the bug? even your timeout is adapted to 
the test time factory, but the solution still depends to a fixed timeout 
and a  fixed line out.


Shanliang

Jaroslav Bachorik wrote:

Please, review the following test fix:

Issue : https://bugs.openjdk.java.net/browse/JDK-6791551
Webrev: http://cr.openjdk.java.net/~jbachorik/6791551/webrev.00

The fix prevents the situation when the first debuggee has not managed 
to finish its intialization while the second one is started up thus 
making the port available for the second debuggee and failing the test.


The patch is using the library methods to configure and launch the 
debuggee and the test waits for the well known string to appear in the 
first debuggee output before attempting to launch the second debuggee.


Thanks,

-JB-

Re: Codereview request: 6980984 java/lang/management/MemoryMXBean/MemoryManagement is not robust when getMax() returns -1

[shanliang]

Jaroslav Bachorik wrote:

Looks good.

Just a side question - why are you using the divider of 10 (and the 
original divider is 20)?

Did change to 20 in the last version :)

I tested with different dividers and got almost same running time with 
10 and 20.


Thanks,
Shanliang


-JB-

On 22.1.2014 17:29, shanliang wrote:

Here is the new version:
http://cr.openjdk.java.net/~sjiang/JDK-6980984/01/

with the modifications for:
1) synchronization issues raised by Daniel
2) "setUsatgeThreshold" -> "setUsageThreshold"

The odd issue about getting slower with:
chunkSize = 1M;
maybe was from the "old gen" behavior, but that seems not important for
the test.

Thanks,
Shanliang

Daniel Fuchs wrote:

Hi Shanliang,

I just notice that there are some synchronization
issues in the test as well: all static variables
used by the allocator thread should be declared
volatile (chunkSize, mpool ...).

-- daniel

On 1/22/14 2:27 PM, shanliang wrote:

Hi,

The bug was reproduced only on jdk6 on my Mac, but well passed on 
7/8/9.


We can have several solutions, like to use:
Runtime.getRuntime().maxMemory()
Runtime.getRuntime().totalMemory;

MemoryUsage.getCommitted()

or hard-code chunkSize to be 1M.

I found that the test ran much faster with:
chunkSize = Runtime.getRuntime().freeMemory()/10;
than:
chunkSize = 1M;

webrev:
http://cr.openjdk.java.net/~sjiang/JDK-6980984/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-6980984

Thanks,
Shanliang

Re: Codereview request: 6980984 java/lang/management/MemoryMXBean/MemoryManagement is not robust when getMax() returns -1

[shanliang]

Here is the new version:
   http://cr.openjdk.java.net/~sjiang/JDK-6980984/01/

with the modifications for:
1) synchronization issues raised by Daniel
2) "setUsatgeThreshold" -> "setUsageThreshold"

The odd issue about getting slower with:
   chunkSize = 1M;
maybe was from the "old gen" behavior, but that seems not important for 
the test.


Thanks,
Shanliang

Daniel Fuchs wrote:

Hi Shanliang,

I just notice that there are some synchronization
issues in the test as well: all static variables
used by the allocator thread should be declared
volatile (chunkSize, mpool ...).

-- daniel

On 1/22/14 2:27 PM, shanliang wrote:

Hi,

The bug was reproduced only on jdk6 on my Mac, but well passed on 7/8/9.

We can have several solutions, like to use:
Runtime.getRuntime().maxMemory()
Runtime.getRuntime().totalMemory;

MemoryUsage.getCommitted()

or hard-code chunkSize to be 1M.

I found that the test ran much faster with:
chunkSize = Runtime.getRuntime().freeMemory()/10;
than:
chunkSize = 1M;

webrev:
http://cr.openjdk.java.net/~sjiang/JDK-6980984/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-6980984

Thanks,
Shanliang

Re: Codereview request: 6980984 java/lang/management/MemoryMXBean/MemoryManagement is not robust when getMax() returns -1

[shanliang]

Jaroslav Bachorik wrote:

On 22.1.2014 15:32, shanliang wrote:

Jaroslav Bachorik wrote:

Hi Shanliang,
On 22.1.2014 14:27, shanliang wrote:

Hi,

The bug was reproduced only on jdk6 on my Mac, but well passed on 
7/8/9.


We can have several solutions, like to use:
Runtime.getRuntime().maxMemory()
Runtime.getRuntime().totalMemory;

MemoryUsage.getCommitted()


You were also mentioning passing -XMaxHeapSize argument to force G1 to
put restriction on the max heap size in the issue. Does it work?

Yes it worked too, as other solutions I mentioned above.




or hard-code chunkSize to be 1M.

I found that the test ran much faster with:
chunkSize = Runtime.getRuntime().freeMemory()/10;
than:
chunkSize = 1M;


That's odd - unless "Runtime.getRuntime().freeMemory()/10" gives you a
chunk much smaller than 1M. BTW, why do you divide the amount of free
memory by 10? The "mu.getMax()" based calculation is using 20.
Wouldn't this give you a bigger chunk when calculating it from
"freeMemory()". And this would also increase the expected threshold
since the chunk is multiplied by NUM_CHUNKS to get the threshold.

Yes it is interesting, it took 15 seconds and num_iteration = 145 with
chunkSize = 1M, but with Runtime.getRuntime().freeMemory()/10, it worked
as without G1GC, took less 1 second and num_iteration = 2, at least on
my Mac.


Just out of curiosity - what is the chunk size when calculating via 
"Runtime.getRuntime().freeMemory()/10"?

8325091


-JB-




And just a nits:
1. While you are changing the source you might fix the type at L28
"setUsatgeThreshold" -> "setUsageThreshold"

OK, good catch.

2. Should the @bug annotation contain the test issues? Just asking - I
saw fixes without the test issue number added to the @bug annotation.

Not sure, but remember that once I was suggested to add it.

Thanks,
Shanliang


Cheers,

-JB-



webrev:
http://cr.openjdk.java.net/~sjiang/JDK-6980984/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-6980984

Thanks,
Shanliang

Re: Codereview request: 6980984 java/lang/management/MemoryMXBean/MemoryManagement is not robust when getMax() returns -1

[shanliang]

Jaroslav Bachorik wrote:

Hi Shanliang,
On 22.1.2014 14:27, shanliang wrote:

Hi,

The bug was reproduced only on jdk6 on my Mac, but well passed on 7/8/9.

We can have several solutions, like to use:
Runtime.getRuntime().maxMemory()
Runtime.getRuntime().totalMemory;

MemoryUsage.getCommitted()


You were also mentioning passing -XMaxHeapSize argument to force G1 to 
put restriction on the max heap size in the issue. Does it work?

Yes it worked too, as other solutions I mentioned above.




or hard-code chunkSize to be 1M.

I found that the test ran much faster with:
chunkSize = Runtime.getRuntime().freeMemory()/10;
than:
chunkSize = 1M;


That's odd - unless "Runtime.getRuntime().freeMemory()/10" gives you a 
chunk much smaller than 1M. BTW, why do you divide the amount of free 
memory by 10? The "mu.getMax()" based calculation is using 20. 
Wouldn't this give you a bigger chunk when calculating it from 
"freeMemory()". And this would also increase the expected threshold 
since the chunk is multiplied by NUM_CHUNKS to get the threshold.
Yes it is interesting, it took 15 seconds and num_iteration = 145 with 
chunkSize = 1M, but with Runtime.getRuntime().freeMemory()/10, it worked 
as without G1GC, took less 1 second and num_iteration = 2, at least on 
my Mac.



And just a nits:
1. While you are changing the source you might fix the type at L28 
"setUsatgeThreshold" -> "setUsageThreshold"

OK, good catch.
2. Should the @bug annotation contain the test issues? Just asking - I 
saw fixes without the test issue number added to the @bug annotation.

Not sure, but remember that once I was suggested to add it.

Thanks,
Shanliang


Cheers,

-JB-



webrev:
http://cr.openjdk.java.net/~sjiang/JDK-6980984/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-6980984

Thanks,
Shanliang

Codereview request: 6980984 java/lang/management/MemoryMXBean/MemoryManagement is not robust when getMax() returns -1

[shanliang]

Hi,

The bug was reproduced only on jdk6 on my Mac, but well passed on 7/8/9.

We can have several solutions, like to use:
   Runtime.getRuntime().maxMemory()
   Runtime.getRuntime().totalMemory;

   MemoryUsage.getCommitted()

or hard-code chunkSize to be 1M.

I found that the test ran much faster with:
   chunkSize = Runtime.getRuntime().freeMemory()/10;
than:
   chunkSize = 1M;

webrev:
http://cr.openjdk.java.net/~sjiang/JDK-6980984/00/

bug:
https://bugs.openjdk.java.net/browse/JDK-6980984

Thanks,
Shanliang

Re: RFR 8019389: SA-JDI JSR292: sun.jvm.hotspot.jdi.StackFrame.thisObject() throws sun.jvm.hotspot.utilities.AssertionFailure: sanity check

[shanliang]

Olivier,

Now it is 2014 :)


Olivier Lagneau wrote:

Please review the following simple fix.

Issue: https://bugs.openjdk.java.net/browse/JDK-8019389
Webrev: http://cr.openjdk.java.net/~olagneau/8019389/webrev.00/ 



The issue is due to the fact that _invokeHandle bytecode is generated 
by hotspot,
but is not declared in agent code. Just declaring the new bytecode 
solves the assertion failure.


However the tests reported in 8019389 
(bootstrapOtherStratumInStackTrace, targetOtherStratumInStackTrace)
suffer the problem from  JDK-7016268 
 : Can't get strata 
information through SA-JDI
Thus, the "stratum mismatch" related to JDK-7016268 will still be 
present after fix.

This second problem has to be fixed through JDK-7016268.

Thanks,
Olivier.