Hi Chris,
On 2/2/2016 8:58 AM, Chris Hegarty wrote:
I think this API should be a good replacement for sun.misc.Signal.
Some comments:
- The second @implNote, referring to s.m.Signal, I assume can be removed?
yes, except that sun.misc.Signal is still around for 9; there there
should be note somewhere about the
interactions, perhaps it should be in s.m.Signal instead.
- I agree with Peter, about the permission regarding the thread that executes
the consumer. Does it make sense to support a ThreadFactory, or may that
is overkill.
Using InnocuousThread should be sufficient.
Adding a ThreadFactory to the register method would give more control
but is probably more than necessary.
- Is it necessary for ‘raise' to declare that it may throw UOE? Since the
Signal
and impl are already retrieved. Or maybe there is another possibility of
failure?
As currently implemented in the VM, some signals can be found and
handled but not raised.
So UOE might occur on raise even if the signal can be handled.
- Signal.dispatch just calls SignalImpl.dispatch. Should the VM just do this
directly?
Yes, but since the VM has a hard coded class/method entry point; I
thought it would be be lower
maintenance to keep it in a well known class. (I'm open to other
registration mechanisms for the callback too).
I suspect the VM folks want to own that initial Signal Dispatch thread
or I would propose just a blocking call to native
to get the next signal; it returns when there is a signal to deliver.
But that change can be factored out for later
if it is a good idea.
- unregister is based on the consumer, possibly lambda’s, identity ? This
seems
strange.
Unless I'm mistaken, lambda's have identity and if the caller needs to
unregister it, they would need to save
the reference. Using an explicit class implementing Consumer<Signal>
would ensure that.
- I’m still confused by the registerDefault, and I’m not sure if it is
actually needed.
Why can Terminator not just register as normal?
The next register would overwrite the handler and the implementation
would not know that
that handler should be the one restored when a client (like jshell)
unregisters its control-c handler.
- Should the instance methods that register/unregister/raise perform a
security check ( in case the Signal escapes, for example in the consumer )
The Signal is a capability, if you have a Signal reference you have the
power; similar to Process and ProcessHandle
the security check is done to get the reference to begin with. From an
auditing point of view,
there is no ambiguity in code that has access to a Signal reference; if
has the reference it can control
the process.
(I haven't done a review with the security folks on this yet).
Thanks, Roger
-Chris.
On 1 Feb 2016, at 16:02, Roger Riggs <roger.ri...@oracle.com> wrote:
Please review an API addition to handle signals such as SIGINT, SIGHUP, and
SIGTERM.
This JEP 260 motivated alternative to sun.misc.Signal supports the use case for
interactive applications that need to handle Control-C and other signals.
The new java.util.Signal class provides a settable primary signal handler and a
default
signal handler. The primary signal handler can be unregistered and handling is
restored
to the default signal handler. System initialization registers default signal
handlers
to terminate on SIGINT, SIGHUP, and SIGTERM. Use of the Signal API requires
a permission if a SecurityManager is set.
The sun.misc.Signal implementation is modified to be layered on a common
thread and dispatch mechanism. The VM handling of native signals is not
affected.
The command option to reduce signal use by the runtime with -Xrs is unmodified.
The changes to hotspot are minimal to rename the hardcoded callback to the Java
Signal dispatcher.
Please review and comment on the API and implementation.
javadoc:
http://cr.openjdk.java.net/~rriggs/signal-doc/
Webrev:
jdk: http://cr.openjdk.java.net/~rriggs/webrev-signal-8087286/
hotspot: http://cr.openjdk.java.net/~rriggs/webrev-hs-signal-8087286/
Issue:
https://bugs.openjdk.java.net/browse/JDK-8087286
JEP 260:
https://bugs.openjdk.java.net/browse/JDK-8132928
Thanks, Roger
