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.lev...@gmail.com
<mailto:peter.lev...@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.lev...@gmail.com <mailto:peter.lev...@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 -> ...
