RFR 9: JDK-8155102: Process.toString could include pid, isAlive, exitStatus

[Roger Riggs]

Hi Andrey,

I added a test to test/java/lang/ProcessBuilder/Basic.java.
Every change must have a test.

    http://cr.openjdk.java.net/~rriggs/webrev-process-8155102/

I can handle the extension request.

Addition review comments welcome.

Roger


On 8/2/2016 1:30 AM, Andrey Dyachkov wrote:
> 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 
> <mailto: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
>>     <mailto: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