JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix

[Chris Plummer]

On 12/15/17 7:18 AM, 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;
>      }
>
You're explanation sounds reasonable, but I'm not clear on the code 
changes. What's the difference between "path" and "socket_name"? Maybe 
if I saw the diff in context it would be more apparent.

Chris