Hi,
On 2/1/2016 3:17 PM, Gerard Ziemski wrote:
On Feb 1, 2016, at 1:25 PM, Roger Riggs <roger.ri...@oracle.com> wrote:
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.
Right, but you don’t actually answer why we don’t allow unknown (to us at the
moment) signals. Why have a limit in place, unless there is a good reason?
There is no artificial limit; just the list of signal names. The
implementation should not be expected to
handle unknown inputs.
#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.
The proposed Signal “feels” incomplete to me without this, though I understand
that it meets the original goal. I would love to see at the very least a
followup enhancement filed to address this.
how about:
4914493 (process) Cannot send arbitary signals to UNIX process
<https://bugs.openjdk.java.net/browse/JDK-4914493>
#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.
If it doesn’t matter, the why not just use constructor “Signal signal = new
Signal(“SIGINT”)” ?
Factory methods are preferred to constructors; it allows greater
flexibility in the implementation, current and future.
#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.
Isn’t it also true for register/unregister?
Register is a strong takeover of responsibility for handling the signal.
Unregister is safer if the caller has to say which handler to remove and
not remove one the caller
did not register.
In practice, it requires a higher level coordination to avoid
interference between competing
interests in handling signals which makes it more complicated than
necessary for the use case.
#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.
I was hoping that behavior is all that’s needed.
Worth considering, will look for other comments on the subject.
Thanks, Roger
Thanks for the review and comments, Roger
Thanks for all the work!
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
