[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Uni
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2017-12-15 20:06:25
Message-ID: 152e69b0-183f-e98b-fb82-88ccc6a36180 () oracle ! com
[Download RAW message or body]
Hi Gary,
This minimal solution looks reasonable to me.
The fix looks good too.
The whole context of this situation is not clear to me yet.
Why the /tmp/.java_pid<PID> socket file could not be opened?
Is it a lack of permissions or something else?
Is it possible to reproduce this with a test?
If it is not easy then I'm Ok to push the fix as it is.
What tests do you run to make sure there is no regression?
Thanks,
Serguei
On 12/15/17 07:18, Gary Adams wrote:
> On 12/15/17, 9:14 AM, Daniel D. Daugherty wrote:
>> On 12/15/17 9:05 AM, Gary Adams wrote:
>>> Looking into some minor bugs in the attach area for the next release.
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8188856
>>>
>>> Seems like a minor change to fix this error message that mentions
>>> the java_pid socket file,
>>> but then outputs the attach_pid file when the socket file is not
>>> created .
>>>
>>> What is the general preference for this type fix?
>>> - remove the reference to the "socket file" in the error message
>>> - construct the filename for the socket that was not found
>>
>> Just an opinion...
>>
>> Having the specific pathname for the <socket_file> that cannot be
>> found is a better diagnostic. Perhaps it's possible to refactor the
>> code that generates the <socket_file> path so that both sides can
>> share that function to generate the same name...
>>
>> It might not be possible to do this sharing, e.g., one side might
>> be in Java and the other side might be in C... In that case, having
>> the same algorithm implemented in both places with comments to link
>> the two would be my next choice...
>>
>> Of course, you have to balance this level of effort against other
>> things on your plate...
>>
>> Dan
>>
> So one minimal solution would be to save the socket file name, so
> it's available when the error message is created.
>
> diff --git
> a/src/jdk.attach/aix/classes/sun/tools/attach/VirtualMachineImpl.java
> b/src/jdk.attach/aix/classes/sun/tools/attach/VirtualMachineImpl.java
> --- a/src/jdk.attach/aix/classes/sun/tools/attach/VirtualMachineImpl.java
> +++ b/src/jdk.attach/aix/classes/sun/tools/attach/VirtualMachineImpl.java
> @@ -47,8 +47,9 @@
> // Any changes to this needs to be synchronized with HotSpot.
> private static final String tmpdir = "/tmp";
>
> - // The patch to the socket file created by the target VM
> + // The path to the socket file created by the target VM
> String path;
> + String socket_name;
>
> /**
> * Attaches to the target VM
> @@ -98,7 +99,7 @@
> throw new AttachNotSupportedException(
> String.format("Unable to open socket file %s:
> " +
> "target process %d doesn't respond within
> %dms " +
> - "or HotSpot VM not loaded", f.getPath(),
> pid, time_spend));
> + "or HotSpot VM not loaded", socket_name,
> pid, time_spend));
> }
> } finally {
> f.delete();
> @@ -267,10 +268,11 @@
> // Return the socket file for the given process.
> private String findSocketFile(int pid) {
> File f = new File(tmpdir, ".java_pid" + pid);
> + socket_name = f.getPath();
> if (!f.exists()) {
> return null;
> }
> - return f.getPath();
> + return socket_name;
> }
>
> // On Solaris/Linux/Aix a simple handshake is used to start the
> attach mechanism
> diff --git
> a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
> b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
> ---
> a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
> +++
> b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
> @@ -48,8 +48,9 @@
> // Any changes to this needs to be synchronized with HotSpot.
> private static final String tmpdir = "/tmp";
>
> - // The patch to the socket file created by the target VM
> + // The path to the socket file created by the target VM
> String path;
> + String socket_name;
>
> /**
> * Attaches to the target VM
> @@ -102,7 +103,8 @@
> throw new AttachNotSupportedException(
> String.format("Unable to open socket file %s:
> " +
> "target process %d doesn't respond within
> %dms " +
> - "or HotSpot VM not loaded", f.getPath(),
> pid, time_spend));
> + "or HotSpot VM not loaded", socket_name, pid,
> + time_spend));
> }
> } finally {
> f.delete();
> @@ -275,10 +277,11 @@
> // procfs regardless of namespaces.
> String root = "/proc/" + pid + "/root/" + tmpdir;
> File f = new File(root, ".java_pid" + ns_pid);
> + socket_name = f.getPath();
> if (!f.exists()) {
> return null;
> }
> - return f.getPath();
> + return socket_name;
> }
>
> // On Solaris/Linux a simple handshake is used to start the
> attach mechanism
> diff --git
> a/src/jdk.attach/macosx/classes/sun/tools/attach/VirtualMachineImpl.java
> b/src/jdk.attach/macosx/classes/sun/tools/attach/VirtualMachineImpl.java
> ---
> a/src/jdk.attach/macosx/classes/sun/tools/attach/VirtualMachineImpl.java
> +++
> b/src/jdk.attach/macosx/classes/sun/tools/attach/VirtualMachineImpl.java
> @@ -46,8 +46,9 @@
> // Any changes to this needs to be synchronized with HotSpot.
> private static final String tmpdir;
>
> - // The patch to the socket file created by the target VM
> + // The path to the socket file created by the target VM
> String path;
> + String socket_name;
>
> /**
> * Attaches to the target VM
> @@ -96,8 +97,9 @@
> if (path == null) {
> throw new AttachNotSupportedException(
> String.format("Unable to open socket file %s:
> " +
> - "target process %d doesn't respond within
> %dms " +
> - "or HotSpot VM not loaded", f.getPath(),
> pid, time_spend));
> + "target process %d doesn't respond within %dms " +
> + "or HotSpot VM not loaded", socket_name,
> + pid, time_spend));
> }
> } finally {
> f.delete();
> @@ -269,7 +271,8 @@
> private String findSocketFile(int pid) {
> String fn = ".java_pid" + pid;
> File f = new File(tmpdir, fn);
> - return f.exists() ? f.getPath() : null;
> + socket_name = f.getPath();
> + return f.exists() ? socket_name : null;
> }
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic