RFR 9: 8087286 Need a way to handle control-C and possibly some other signals

Tue Feb 2 16:35:15 UTC 2016

Hi Chris,

On 2/2/2016 11:12 AM, Chris Hegarty wrote:
> Roger,
>
> On 2 Feb 2016, at 15:31, Roger Riggs <Roger.Riggs at Oracle.com> wrote:
>
>> 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.
> Yes, I think it should be in s.m.Signal.
>
>>>   - 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.
> Right, but then it should be specified.
ok
>
>> Adding a ThreadFactory to the register method would give more control but is probably more than necessary.
> Hmmm… maybe. But then it is very restrictive, unless there is some mechanism
> to override it.
same as current s.m.Signal implementation.
>
>>>   - 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.
> Ok, thanks.
>
>>>   - 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.
> Agreed.
>
>>>   - 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.
> Yes, they do, I’m just questioning whether this is what we want.
The handler needs some identity to be able to unregister it,  either the 
method returns some kind of
unique handle for the registration or it uses the identity of the 
consumer itself.
It is simpler to hold on to the reference of the consumer.
>
>>>   - 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.
> So the use case is to register a handler and later restore the previously
> set handler.  It is not possible for register to return the previously registered
> handler, and then it is up the user to store it and reset it, as necessary.
> Rather than the default notion.
Returning the previously set handler exposes a reference to some other 
subsystem's object.
In some cases it might be a security risk.
>
>>>   - 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,
> It is in the new API, but not in s.m.Signal ( it is just pair of string name and number ).
>
>> 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.
> There is a potential issue when the Signal is delivered to the consumer when it
> is raised, if, there is any thread, other than an innocuous thread, executing the
> consumer.
>
> Another small implementation question: is it still necessary for s.m.Signal.dispatch
> to start a new Thread ?
If it re-uses the VM's Signal Dispatch thread, the latency in the signal 
consumer might delay other signal
handling.  Like with the Cleaner threads and functions, there is a 
concern about the impact of one of the clients
on the shared resource (Thread). Starting a new thread allows the impact 
to be reduced.

There might be an argument for using the a common executor but it might 
also introduce additional
latency in processing the signal depending on the state of the executor.
Starting a new thread has more predictable behavior.

Thanks, Roger

>
> -Chris.
>
>> (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.Riggs at 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
>>>>
>>>>
>>>>