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

[Roger Riggs]

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