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

[Rob McKenna]

Great to see this getting in. Thanks Peter!

     -Rob

On 12/05/14 16:51, Peter Levart wrote:
> 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 -> ...
>>
>>
>