> On Feb 1, 2016, at 1:25 PM, Roger Riggs <[email protected]> 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?
>> #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.
>>
>>
>> #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”)” ?
>>
>>
>> #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?
>
>>
>>
>> #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.
>
> Thanks for the review and comments, Roger
Thanks for all the work!
>
>>
>>
>> cheers
>>
>>
>>> On Feb 1, 2016, at 10:02 AM, Roger Riggs <[email protected]>
>>> 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
>>>
>>>
>>>
>
