Hi Andrew,

Redirecting back to serviceability-dev@openjdk.java.net, it is the better list for this topic.

I don't know these tests but it seems odd to stick that assert into every
callback to packetFromThisVMReceived. Perhaps someone more familiar can review and sponsor.

Thanks, Roger




On 6/26/2017 10:37 AM, Andrew Leonard wrote:
Hi Roger,
I have now updated to the latest openjdk9 and examined the existing jdp tests. I have added to those tests to assert the JDP packet processId, which was in fact "null", so it was failing to set it before... Hence, with this testcase update they fail without my patch, and succeed with the new patch.
Being new to this contribution process, where do I go from here please?
Thanks
Andrew

diff -r 2425838cfb5e src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java --- a/src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java Fri Jun 23 14:32:59 2017 -0400 +++ b/src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java Mon Jun 26 15:31:15 2017 +0100
@@ -133,20 +133,8 @@

     // Get the process id of the current running Java process
     private static Integer getProcessId() {
-        try {
-  // Get the current process id using a reflection hack
-  RuntimeMXBean runtime = ManagementFactory.getRuntimeMXBean();
-  Field jvm = runtime.getClass().getDeclaredField("jvm");
-  jvm.setAccessible(true);
-
-  VMManagement mgmt = (sun.management.VMManagement) jvm.get(runtime);
-  Method pid_method = mgmt.getClass().getDeclaredMethod("getProcessId");
-  pid_method.setAccessible(true);
-  Integer pid = (Integer) pid_method.invoke(mgmt);
-  return pid;
-        } catch(Exception ex) {
-  return null;
-        }
+        // Get the current process id
+        return (int)ProcessHandle.current().pid();
     }

diff -r 2425838cfb5e test/sun/management/jdp/JdpOnTestCase.java
--- a/test/sun/management/jdp/JdpOnTestCase.java Fri Jun 23 14:32:59 2017 -0400 +++ b/test/sun/management/jdp/JdpOnTestCase.java Mon Jun 26 15:30:51 2017 +0100
@@ -31,6 +31,7 @@

 import java.net.SocketTimeoutException;
 import java.util.Map;
+import static jdk.testlibrary.Asserts.assertNotEquals;

 public class JdpOnTestCase extends JdpTestCase {

@@ -58,6 +59,7 @@
         final String jdpName = payload.get("INSTANCE_NAME");
log.fine("Received correct JDP packet #" + String.valueOf(receivedJDPpackets) +
     ", jdp.name=" + jdpName);
+ assertNotEquals(null, payload.get("PROCESS_ID"), "Expected payload.get(\"PROCESS_ID\") to be not null.");
     }




Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com




From: Roger Riggs <roger.ri...@oracle.com>
To: Andrew Leonard <andrew_m_leon...@uk.ibm.com>
Cc: core-libs-...@openjdk.java.net
Date: 23/06/2017 13:57
Subject: Re: Proposal:JdpController.getProcessId() VM compatibility improvement
------------------------------------------------------------------------



Hi Leonard,

No need to stray from your intended focus.
The other is just another cleanup.

Thanks, Roger


On 6/23/17 6:57 AM, Andrew Leonard wrote:
Thanks Roger,
So yes that issue does look similar, although I was only looking in the management interfaces. I'll have a peek at those references to see if they are similar, it would make sense to clean those up too, to be consistent. Your thoughts? I'll also see if there's a junit for this method, if not i'll see if I can add one.
Cheers
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: _andrew_m_leon...@uk.ibm.com_ <mailto:andrew_m_leon...@uk.ibm.com>




From: Roger Riggs _<roger.ri...@oracle.com>_ <mailto:roger.ri...@oracle.com> To: _core-libs-...@openjdk.java.net_ <mailto:core-libs-...@openjdk.java.net>
Date: 22/06/2017 18:03
Subject: Re: Proposal:JdpController.getProcessId() VM compatibility improvement Sent by: "core-libs-dev" _<core-libs-dev-boun...@openjdk.java.net>_ <mailto:core-libs-dev-boun...@openjdk.java.net>
------------------------------------------------------------------------



HI,

This looks like:

JDK-8074569 <_https://bugs.openjdk.java.net/browse/JDK-8074569_> Update
implementation to use ProcessHandle.current() to get the PID

I'm happy to sponsor.

Roger

On 6/22/2017 12:27 PM, Andrew Leonard wrote:
> Thanks Alan,
> Yes, you're right the exception swallowing can be removed too.
> I'll re-post to serviceability-dev...
> Cheers
> Andrew
>
> Andrew Leonard
> Java Runtimes Development
> IBM Hursley
> IBM United Kingdom Ltd
> Phone internal: 245913, external: 01962 815913
> internet email: _andrew_m_leon...@uk.ibm.com_ <mailto:andrew_m_leon...@uk.ibm.com>
>
>
>
>
> From: Alan Bateman _<alan.bate...@oracle.com>_ <mailto:alan.bate...@oracle.com> > To: Andrew Leonard _<andrew_m_leon...@uk.ibm.com>_ <mailto:andrew_m_leon...@uk.ibm.com>,
> _core-libs-...@openjdk.java.net_ <mailto:core-libs-...@openjdk.java.net>
> Date:   22/06/2017 17:21
> Subject: Re: Proposal:JdpController.getProcessId() VM compatibility
> improvement
>
>
>
>
> This seems a good clean-up, esp. as java.management doesn't open
> sun.management to jdk.management.agent so I'll bet this wasn't working
> any. I assume the exception swallowing can be removed too.
>
> Can you bring this to serviceability-dev as this is where this code is
> maintained?
>
> On 22/06/2017 14:34, Andrew Leonard wrote:
>> Hello,
>> I would like to propose the change below to the
>> JdpController.getProcessId() method which currently uses a reflection
> hack
>> and the VMManagement interface. Instead for JDK9+ we can use the
>> ProcessHandle.current().getPid() method, which will also allow better
>> compatibility with other pluggable VMs.
>>
>>
> jdk/src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java
>> 36d35
>> < import sun.management.VMManagement;
>> 137,146c136,137
>> <             // Get the current process id using a reflection hack
>> <             RuntimeMXBean runtime =
>> ManagementFactory.getRuntimeMXBean();
>> <             Field jvm = runtime.getClass().getDeclaredField("jvm");
>> <             jvm.setAccessible(true);
>> <
>> <             VMManagement mgmt = (sun.management.VMManagement)
>> jvm.get(runtime);
>> <             Method pid_method =
>> mgmt.getClass().getDeclaredMethod("getProcessId");
>> <             pid_method.setAccessible(true);
>> <             Integer pid = (Integer) pid_method.invoke(mgmt);
>> <             return pid;
>> ---
>>>               // Get the current process id
>>>               return (int)ProcessHandle.current().getPid();
>> I'd appreciate any feedback please, and how I would go about obtaining a
>> sponsor and contributor?
>>
>> Thanks
>> Andrew
>>
>> Andrew Leonard
>> Java Runtimes Development
>> IBM Hursley
>> IBM United Kingdom Ltd
>> Phone internal: 245913, external: 01962 815913
>> internet email: _andrew_m_leon...@uk.ibm.com_ <mailto:andrew_m_leon...@uk.ibm.com>
>>
>> Unless stated otherwise above:
>> IBM United Kingdom Limited - Registered in England and Wales with number
>> 741598.
>> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6
> 3AU
>
>
>
>
> Unless stated otherwise above:
> IBM United Kingdom Limited - Registered in England and Wales with number
> 741598.
> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU




Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU



Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Reply via email to