RFR: 8000975: (process) Merge UNIXProcess.java.bsd & UNIXProcess.java.linux (& .solaris & .aix)

[Peter Levart]

On 05/07/2014 11:04 PM, Martin Buchholz wrote:
> Hi Peter,
>
> Your last version version looks very good.  Approved!  I'm reduced to 
> asking you to fix ancient buglets of mine.
>
> *"I'd spell creat with an e.*"
>
>
>   188      * Create a process. Depending on the mode flag, this is done by
> *"I'd spell Create with an s.*"
>
>
>

Alan, Martin, Roger, thank you for your patience in reviewing this. 
Thanks also to Paul, David M., Rob and Volker for comments and testing. 
I have just pushed this changeset to jdk9-dev with a little javadoc 
tweak on a private native method suggested by Martin, fixing also the 
description of the 'mode' parameter to be in-sync with what it really 
means in native code.

We can take this as a basis for consolidating some more code after some 
bake-in time...

Regards, Peter

>
>
>
> On Wed, May 7, 2014 at 12:42 AM, Peter Levart <peter.levart at gmail.com 
> <mailto:peter.levart at gmail.com>> wrote:
>
>     Hi Martin,
>
>     I have restructured the processReaperExecutor construction. It now
>     incorporates system thread group search and thread pool
>     construction in one doPrivileged call. I also extracted the
>     creation of ThreadFactory into a local variable so it's more
>     explicit now. Here's the webrev:
>
>     http://cr.openjdk.java.net/~plevart/jdk9-dev/UNIXProcess/webrev.09/ <http://cr.openjdk.java.net/%7Eplevart/jdk9-dev/UNIXProcess/webrev.09/>
>
>     ...java/lang/ProcessBuilder tests still pass.
>
>     Regards, Peter
>
>     P.S. I don't belive Executors.newCachedTreadPool() could
>     strengthen security in future, since this would break existing
>     user code. The javadoc does not specify any SecurityExceptions.
>     But anyway - wrapping the whole logic of processReaperExecutor
>     construction in one doPrivileged call allowed me to use local
>     variables instead of private static final fields for
>     systemThreadGroup and threadFactory, so this looks nicer too.
>
>
>
>     On 05/06/2014 07:41 PM, Martin Buchholz wrote:
>>
>>
>>
>>     On Sun, May 4, 2014 at 3:26 AM, Peter Levart
>>     <peter.levart at gmail.com <mailto:peter.levart at gmail.com>> wrote:
>>
>>
>>         Ah, I've forgotten to mention the most important change from
>>         webrev.07.
>>
>>         Original code wraps the processReaperExecutor construction
>>         into the doPrivileged() call. I think this was not needed.
>>
>>         The Executors.newCachedThreadPool() does not need any special
>>         privileges. And construction of nested class
>>         ProcessReaperThreadFactory also didn't need any special
>>         privileges - apart from static initialization which called
>>         the getRootThreadGroup() method. But that method did it's own
>>         doPrivileged() wrapping in order to be able to climb the
>>         ThreadGroup hierarchy (which requires privilege). In
>>         webrev.07 I followed original code and wrapped the
>>         construction of a processReaperExecutor into a doPrivileged()
>>         although in webrev.07 the rootThreadGroup search happens as
>>         part of UNIXProcess static initialization and is already
>>         wrapped in doPrivileged(). In webrev.08 I removed this
>>         superfluous doPrivileged(). I think this is correct.
>>         SecurityManagerClinit test passes.
>>
>>
>>     Although I think you're right with the current implementation, it
>>     seems too brittle to me to remove the doPrivileged, since you
>>     have no idea what future changes to newCachedThreadPool() might
>>     do (or what other JDK implementations might do); notably it might
>>     instantiate some kind of micro-manager thread or do some other
>>     privileged operation.
>>
>>     If you want to have only one call to doPrivileged, you can cause
>>     both rootThreadGroup and processReaperExecutor to be initialized
>>     in one static block, although you will need to jump through some
>>     hoops to keep these fields final.
>>
>>     ---
>>
>>     As it stands,
>>
>>     Executors.newCachedThreadPool(grimReaper -> {
>>
>>     doesn't mention either the type ThreadFactory or Runnable.
>>
>>     It might be clearer (more verbosity in the spirit of Java) if you
>>     used explicit ThreadFactory type
>>
>>     ThreadFactory factory = grimReaper -> ...
>
>