Hi Gerard,
On 2/1/2016 1:58 PM, Gerard Ziemski wrote:
hi Roger,
I love that we are adding this Signal object. I have several questions, but
please do not take them as criticism, I’m just seeking clarification and
providing feedback:
#1 Re: "Consumers for signals that are unknown or reserved by the Java
implementation can not be registered.”
- Why don't we want to allow unknown signals? This will make us have to update
our implementation if we want to support new or platform specific signals in
the future.
The statement was aimed primarily at the Java Signal API; there is quite
a bit of detail
oriented code in the VM to initialize and handle signals. Most of it is
agnostic to the signal number
and would just pass it through. If a signal is not supported by the OS
(think SIGHUP on Windows)
that should bubble up as being not available. The 'cannot be
registered' might be re-worded to say
it throws an exception, as the method javadoc does.
The set of signals is a pretty slow moving target so updating
implementations should not be a big issue.
#2 Re: "java.util.Signal.raise()”
- That API raises the signal in the current process, but what about sending a
signal to another process for interprocess communication?
I left that for a separate issue but would be a straight-forward
addition to java.lang.ProcessHandle/Process.
#3 Re: "Signal.of("SIGINT”)”
- Is this a factory method that returns the same object if called more than
once?
Maybe, maybe not, why would it matter.
The real state is encapsulate is in the SignalImpl instances which are
singletons per signal.
I was trying to keep the Signal object stateless to allow it to be a
value-type and lighter weight
some day.
#4 Re: "public boolean unregister(Consumer<Signal> consumer)”
- Why is this API returning a value? Wouldn’t having a Signal API like “public
Consumer<Signal> getConsumer()” be more flexible?
The return value reports whether it unregistered the specific consumer.
If it was not
the concurrently registered the caller might want to know it was
currently registered.
I expect the return would mostly be ignored.
The getConsumer()/unregister consumer pair would be vulnerable to race
conditions
and require some external locking to get predictable behavior.
#5 Re: "public void registerDefault(Consumer<Signal> consumer)”
- Do we really need this API? Can’t the same be achieved with the plain vanilla
“public void register(Consumer<Signal> consumer)” I guess I don’t really see
what makes this API special.
The java runtime currently registers termination handler to cleanly
shutdown if someone types control-c.
It is useful to be able to remove the application provided signal
handler and be able to restore the
system defaults.
This API could be hidden as a pure implementation detail. So
unregistering the signal handler
would always restore the appropriate system ones.
Thanks for the review and comments, Roger
cheers
On Feb 1, 2016, at 10:02 AM, 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
