Improve registering signal handlers in java.lang.Terminator.setup()

[David Holmes]

On 7/08/2012 1:03 PM, Frank Ding wrote:
> Hi Holmes,

David :)

> It is not only when -Xrs is specified. The circumstances can be
> illustrated below
> 1. a JVM has its own extra signal registration that occupies say, "INT",
> prior to the execution Terminator.setup().
> 2. User customizes HotSpot to have similar overridden signal handling.
> In both cases, I think Terminator.setup() should work at best effort to
> register all.

The change is harmless but as far as I can see you would have to 
customize the VM before this change would have any affect. The signals 
involved here are the SHUTDOWNn_SIGNAL values (which for linux/solaris 
are HUP, INT and TERM). JVM_RegisterSignal simply does:

     case SHUTDOWN1_SIGNAL:
     case SHUTDOWN2_SIGNAL:
     case SHUTDOWN3_SIGNAL:
       if (ReduceSignalUsage) return (void*)-1;
       if (os::Linux::is_sig_ignored(sig)) return (void*)1;
   }

So with -Xrs all values will get rejected as a group. As far as I can 
see this is the only time that -1 will be returned to Signal.handle0 and 
hence the only circumstance where IllegalArgumentException will be 
thrown. So without customizing the VM if any of these signals cause 
IllegalArgumentException then they all will and hence having distinct 
try/catch blocks for each is pointless.

Did I miss something?

David
-----


> Best regards,
> Frank
>
> On 8/6/2012 7:08 PM, David Holmes wrote:
>> On 6/08/2012 6:57 PM, Frank Ding wrote:
>>> Hi Holmes,
>>> I agree with your comment on duplicated comment. It should be
>>> factored out.
>>> The context of the change is that the group of signal handlers should be
>>> registered at best effort, which means for all JVM, we should try to
>>> register them all in an independent way. In fact, the original code does
>>> not act homogeneously. For example, in
>>> solaris/classes/java/lang/Terminator.java, there are 3 registration
>>> calls for signal "HUP", "INT" and "TERM". On start up of some JVM, "INT"
>>> may already have been occupied for some reason and this will cause
>>> "TERM" to be skipped. It is not what the original code intends to do, is
>>> it?
>>
>> My question is: exactly what circumstances will cause the
>> IllegalArgumentException to be thrown? Is it only when -Xrs is
>> specified? And if so does it then affect all of the signals?
>>
>> David
>> ------
>>
>>> I provided a new revision of patch to remove the dup comment, available
>>> @ http://cr.openjdk.java.net/~youdwei/ojdk-430/webrev.01/. Your comment
>>> is much appreciated.
>>>
>>> Thanks and best regards,
>>> Frank
>>>
>>> On 8/6/2012 9:51 AM, David Holmes wrote:
>>>> Hi Frank,
>>>>
>>>> On 3/08/2012 5:39 PM, Frank Ding wrote:
>>>>> Hi guys,
>>>>> I found that in java.lang.Terminator, setup() method,
>>>>> The following code of registering default signal handlers can be
>>>>> improved:
>>>>> / try {
>>>>> Signal.handle(new Signal("INT"), sh);
>>>>> Signal.handle(new Signal("TERM"), sh);
>>>>> } catch (IllegalArgumentException e) {
>>>>> }/
>>>>> The revised code is illustrated below:
>>>>> / try {
>>>>> Signal.handle(new Signal("INT"), sh);
>>>>> } catch (IllegalArgumentException e) {
>>>>> }
>>>>> try {
>>>>> Signal.handle(new Signal("TERM"), sh);
>>>>> } catch (IllegalArgumentException e) {
>>>>> }
>>>>> /The improved version makes more sense since exception thrown from
>>>>> first
>>>>> Signal.handle call does not affect subsequent calls. This is more
>>>>> consistent with its original intention.
>>>>> A patch I made is available @
>>>>> http://cr.openjdk.java.net/~youdwei/ojdk-430/webrev.00//
>>>>>
>>>>> /Could anybody please take a look at it? Thanks in advance/
>>>>
>>>> Can you explain the context for this change. It seems to me that there
>>>> is an expectation that the group of signals act homogenously: all or
>>>> none, whereas your change make it appear that the success/failure for
>>>> each signal is independent. Understanding exactly when/why the
>>>> IllegalArgumentException is thrown is important here.
>>>>
>>>> I don't like seeing the duplicated comment, perhaps that can be
>>>> factored out to the head of the block of code?
>>>>
>>>> Thanks,
>>>> David Holmes
>>>>
>>>>> Best regards,
>>>>> Frank
>>>>> /
>>>>
>>>
>>
>