[PATCH] JDK-8155102: Process.toString could include pid, isAlive, exitStatus

[Andrey Dyachkov]

Roger,

I have changed impl according to your comments, thank you!
Regarding fc-extension, I am not able to add comments in the bug tracker I
think you can do it, correct? Also I am already in OCA list.

diff --git a/src/java.base/unix/classes/java/lang/ProcessImpl.java
b/src/java.base/unix/classes/java/lang/ProcessImpl.java
--- a/src/java.base/unix/classes/java/lang/ProcessImpl.java
+++ b/src/java.base/unix/classes/java/lang/ProcessImpl.java
@@ -630,6 +630,19 @@
         return !hasExited;
     }

+    /**
+     * The {@code toString} method returns a string consisting of
+     * the native process ID of the process and the exit value of the
process.
+     *
+     * @return a string representation of the object.
+     */
+    @Override
+    public String toString() {
+        return new StringBuilder("Process[pid=").append(pid)
+        .append(", exitValue=").append(hasExited ? exitcode : "\"not
exited\"")
+        .append("]").toString();
+    }
+
     private static native void init();

     static {
diff --git a/src/java.base/windows/classes/java/lang/ProcessImpl.java
b/src/java.base/windows/classes/java/lang/ProcessImpl.java
--- a/src/java.base/windows/classes/java/lang/ProcessImpl.java
+++ b/src/java.base/windows/classes/java/lang/ProcessImpl.java
@@ -564,6 +564,20 @@
     private static native boolean isProcessAlive(long handle);

     /**
+     * The {@code toString} method returns a string consisting of
+     * the native process ID of the process and the exit value of the
process.
+     *
+     * @return a string representation of the object.
+     */
+    @Override
+    public String toString() {
+        int exitCode = getExitCodeProcess(handle);
+        return new StringBuilder("Process[pid=").append(getPid())
+                .append(", exitValue=").append(exitCode == STILL_ACTIVE ?
"\"not exited\"" : exitCode)
+                .append("]").toString();
+    }
+
+    /**
      * Create a process using the win32 function CreateProcess.
      * The method is synchronized due to MS kb315939 problem.
      * All native handles should restore the inherit flag at the end of
call.



On Mon, 1 Aug 2016 at 19:34 Roger Riggs <Roger.Riggs at oracle.com> wrote:

> Hi Andrey,
>
> Sorry to be slow in getting back to this.
>
> Thanks for the update;   did you notice that Windows has a separate
> implementation of
> ProcessImpl and would also need a similar, though not identical toString()
> implementation?
>      [jdk/src/java.base/windows/classes/java/lang/ProcessImpl.java ]
>
> Windows has a slightly different implementation of the internal state with
> respect to knowing if it has exited.
> See exitValue() and the native getExitCodeProcess() method.
>
>
> On 7/24/2016 7:42 AM, Andrey Dyachkov wrote:
>
> Hi Roger,
>
> Thank you for reviewing it! I've prepared another version.
>
> diff --git a/src/java.base/unix/classes/java/lang/ProcessImpl.java
> b/src/java.base/unix/classes/java/lang/ProcessImpl.java
> --- a/src/java.base/unix/classes/java/lang/ProcessImpl.java
> +++ b/src/java.base/unix/classes/java/lang/ProcessImpl.java
> @@ -630,6 +630,21 @@
>          return !hasExited;
>      }
>
> +    /**
> +     * The {@code toString} method for class {@code ProcessImpl}
> +     * returns a string consisting of the native process ID of the
> process,
> +     * exit code of the process and the status of the process.
>
> The javadoc for Process uses the terminology "exit value" or "exit
> status"; the usage should be consistent.
>
> +     *
> +     * @return  a string representation of the object.
> +     */
> +    @Override
> +    public String toString() {
> +        return new StringBuilder("ProcessImpl{pid=").append(pid)
>
> The implementation class should be hidden, use "Process".
> Other toString methods use '[]' to enclose the aggregate of the values.
>
> +        .append(", exitcode=").append(exitcode)
>
> 'exitcode=' -> "exitValue" to be consistent with the exitValue method.
>
> This will need an alternate value if the process has not exited, perhaps
> "not exited"
>
> +        .append(", hasExited=").append(hasExited)
>
> This isn't needed with the above 'not exited' string.
>
> +        .append("}").toString();
>
> '}' -> ']'
>
>
> Also, I think this would be viewed more as an enhancement than a bug and
> we'll need
> to request an extension[1][2] for it when the code is complete.
>
> Thanks, Roger
>
> [1] http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-June/004443.html
> [2] http://openjdk.java.net/projects/jdk9/fc-extension-process
>
>
> +    }
> +
>      private static native void init();
>
>      static {
>
> On Mon, 9 May 2016 at 16:50 Roger Riggs <Roger.Riggs at oracle.com> wrote:
>
>> Hi Andrey,
>>
>> Since toString is a public method it needs to provide complete javadoc
>> specification
>> as providing values for debugging information.  ToString methods
>> typically directly
>> reflect the state of the fields of the object.
>>
>> Why should the exitValue have the value of the pid if the process is not
>> alive?
>>
>> Also, the if toString is providing the exitValue then it should be
>> identified as "exitValue", not exitCode.
>> the isAlive value should be identified as "isAlive".
>> The implementation class should not be exposed.  (remove
>> this.getClass().getSimpleName()).
>>
>> The performance of what seems like a simple toString method is not going
>> to be great
>> because of the native calls necessary to determine if the process is
>> alive (called twice)
>> and in the case of exitValue to handle the IllegalStateException.
>>
>> Some of the overhead could be avoided, by implementing toString in each
>> of the ProcessImpl.java
>> files where the current state is known more conveniently with less
>> overhead.
>>
>> $.02, Roger
>>
>>
>> On 5/8/2016 2:47 PM, Andrey Dyachkov wrote:
>> > Hello,
>> >
>> > I have added toString() method in Process.java.
>> >
>> > diff --git a/src/java.base/share/classes/java/lang/Process.java
>> > b/src/java.base/share/classes/java/lang/Process.java
>> > --- a/src/java.base/share/classes/java/lang/Process.java
>> > +++ b/src/java.base/share/classes/java/lang/Process.java
>> > @@ -548,5 +548,16 @@
>> >           return toHandle().descendants();
>> >       }
>> >
>> > +    @Override
>> > +    public String toString() {
>> > +        boolean isAlive = this.isAlive();
>> > +        return new
>> > StringBuilder(this.getClass().getSimpleName()).append("[")
>> > +                        .append("running=").append(isAlive).append(",
>> ")
>> > +                        .append(isAlive ? "pid=" : "exitCode=")
>> > +                        .append(isAlive ? this.getPid() :
>> this.exitValue())
>> > +                        .append("]")
>> > +                        .toString();
>> > +    }
>> > +
>> >
>> >   }
>>
>> --
>
> With great enthusiasm,
> Andrey
>
>
> --

With great enthusiasm,
Andrey