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 -> ...
