RFR 9: JDK-8155102: Process.toString could include pid, isAlive, exitStatus
Roger Riggs
Roger.Riggs at Oracle.com
Fri Aug 5 15:48:11 UTC 2016
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
More information about the core-libs-dev
mailing list