Improve registering signal handlers in java.lang.Terminator.setup()
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/ Best regards, Frank /
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 /
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? 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 /
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 /
Hi Holmes, 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. 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 /
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 /
On 07/08/2012 04:35, David Holmes wrote:
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?
I agree with David and I think this thread is just missing an explanation as to how you ran into this. It this AIX and -Xrs is mapped to a different set of signals, maybe it was an observation when reading the code?? -Alan.
On Tue, 2012-08-07 at 10:15 +0100, Alan Bateman wrote:
On 07/08/2012 04:35, David Holmes wrote:
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?
I agree with David and I think this thread is just missing an explanation as to how you ran into this. It this AIX and -Xrs is mapped to a different set of signals, maybe it was an observation when reading the code??
-Alan.
From a Java Class Library point of view, it seems to me that the desire is to register shutdown hooks for any of these signals (HUP, INT and TERM) whose use has not been restricted by the VM.
So an attempt to register for each of these signals should be made, independent of the result of any of the other attempts. To do anything else builds into the Java class library code assumptions about the behaviour of the VM which are inherently implementation (version?) specific, which is a brittle thing to do. It currently assumes that the VM will either have restricted all these signals (the -Xrs case) or none of them. This assumption holds true for the current version of Hotspot VM, but not necessarily for other VM implementations, whose mix of signal usage may differ. So I think Frank's suggested change helps this code to adhere the VM / Class Library interface boundary, and so makes it less brittle. Regards, Neil -- Unless stated above: IBM email: neil_richards at uk.ibm.com IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
On 07/08/2012 15:02, Neil Richards wrote:
:
From a Java Class Library point of view, it seems to me that the desire is to register shutdown hooks for any of these signals (HUP, INT and TERM) whose use has not been restricted by the VM.
So an attempt to register for each of these signals should be made, independent of the result of any of the other attempts.
To do anything else builds into the Java class library code assumptions about the behaviour of the VM which are inherently implementation (version?) specific, which is a brittle thing to do.
It currently assumes that the VM will either have restricted all these signals (the -Xrs case) or none of them.
This assumption holds true for the current version of Hotspot VM, but not necessarily for other VM implementations, whose mix of signal usage may differ.
So I think Frank's suggested change helps this code to adhere the VM / Class Library interface boundary, and so makes it less brittle.
As David said, the proposed change is harmless and I don't think anyone has any issue with it. It's really just us trying to understand whether there is really an issue here or not as it is has not been clear from the mails so far. I'm guessing it's AIX or J9 where -Xrs may be mapped to a different set of signals. FWIW, the termination setup has not been touched in >10 years. Looking at it now then it could have been done in other ways that wouldn't have been VM specific. Whether it's worth doing this now isn't clear as it just hasn't been an issue (to my knowledge anyway). -Alan
On Tue, 2012-08-07 at 18:01 +0100, Alan Bateman wrote:
On 07/08/2012 15:02, Neil Richards wrote:
:
From a Java Class Library point of view, it seems to me that the desire is to register shutdown hooks for any of these signals (HUP, INT and TERM) whose use has not been restricted by the VM.
So an attempt to register for each of these signals should be made, independent of the result of any of the other attempts.
To do anything else builds into the Java class library code assumptions about the behaviour of the VM which are inherently implementation (version?) specific, which is a brittle thing to do.
It currently assumes that the VM will either have restricted all these signals (the -Xrs case) or none of them.
This assumption holds true for the current version of Hotspot VM, but not necessarily for other VM implementations, whose mix of signal usage may differ.
So I think Frank's suggested change helps this code to adhere the VM / Class Library interface boundary, and so makes it less brittle.
As David said, the proposed change is harmless and I don't think anyone has any issue with it. It's really just us trying to understand whether there is really an issue here or not as it is has not been clear from the mails so far. I'm guessing it's AIX or J9 where -Xrs may be mapped to a different set of signals. FWIW, the termination setup has not been touched in >10 years. Looking at it now then it could have been done in other ways that wouldn't have been VM specific. Whether it's worth doing this now isn't clear as it just hasn't been an issue (to my knowledge anyway).
-Alan
Digging back into its history, I see that it all stems from running java under 'nohup' (e.g. 'nohup java ProgramWithShutdownHooks &'). 'nohup' prevents things being registered against SIGHUP, so trying to do so causes an exception to be thrown. When this happens, the Terminator code currently decides that it shouldn't bother trying to register shutdown hooks for SIGINT or SIGTERM either. The result is that shutdown hooks aren't currently run when a java process run using 'nohup' is sent a SIGINT or SIGTERM signal, because of the code's (false) assumption that a failure to register a handler for SIGHUP must mean that the VM is running in -Xrs mode. I don't think the current observed behaviour in this scenario is either expected or desirable. It occurs for both J9 and Hotspot (at least, on my Ubuntu box, bash shell). Hope this helps to clarify the scenario. Regards, Neil -- Unless stated above: IBM email: neil_richards at uk.ibm.com IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
On 8/08/2012 4:07 AM, Neil Richards wrote:
On Tue, 2012-08-07 at 18:01 +0100, Alan Bateman wrote:
On 07/08/2012 15:02, Neil Richards wrote:
:
From a Java Class Library point of view, it seems to me that the desire is to register shutdown hooks for any of these signals (HUP, INT and TERM) whose use has not been restricted by the VM.
So an attempt to register for each of these signals should be made, independent of the result of any of the other attempts.
To do anything else builds into the Java class library code assumptions about the behaviour of the VM which are inherently implementation (version?) specific, which is a brittle thing to do.
It currently assumes that the VM will either have restricted all these signals (the -Xrs case) or none of them.
This assumption holds true for the current version of Hotspot VM, but not necessarily for other VM implementations, whose mix of signal usage may differ.
So I think Frank's suggested change helps this code to adhere the VM / Class Library interface boundary, and so makes it less brittle.
As David said, the proposed change is harmless and I don't think anyone has any issue with it. It's really just us trying to understand whether there is really an issue here or not as it is has not been clear from the mails so far. I'm guessing it's AIX or J9 where -Xrs may be mapped to a different set of signals. FWIW, the termination setup has not been touched in>10 years. Looking at it now then it could have been done in other ways that wouldn't have been VM specific. Whether it's worth doing this now isn't clear as it just hasn't been an issue (to my knowledge anyway).
-Alan
Digging back into its history, I see that it all stems from running java under 'nohup' (e.g. 'nohup java ProgramWithShutdownHooks&').
'nohup' prevents things being registered against SIGHUP, so trying to do so causes an exception to be thrown.
When this happens, the Terminator code currently decides that it shouldn't bother trying to register shutdown hooks for SIGINT or SIGTERM either.
Thanks Neil that is a much more convincing justification. The "interface is brittle" argument is rather tenous as the key issue is when the underlying method will throw IllegalArgumentException - which is inherently part of the unwritten contract with the native method and thence the VM. The code already assumes that it is okay to ignore the IllegalArgumentException, which implies it knows the exact circumstances when it will be thrown. Anyway the change is okay to push. CR 7189865 created for this. Thanks, David -----
The result is that shutdown hooks aren't currently run when a java process run using 'nohup' is sent a SIGINT or SIGTERM signal, because of the code's (false) assumption that a failure to register a handler for SIGHUP must mean that the VM is running in -Xrs mode.
I don't think the current observed behaviour in this scenario is either expected or desirable. It occurs for both J9 and Hotspot (at least, on my Ubuntu box, bash shell).
Hope this helps to clarify the scenario.
Regards, Neil
Thanks Neil for clarifying it in such a convincing way. And David for creating CR. So now we're ready to commit this patch? Best regards, Frank On 8/8/2012 8:57 AM, David Holmes wrote:
On 8/08/2012 4:07 AM, Neil Richards wrote:
On Tue, 2012-08-07 at 18:01 +0100, Alan Bateman wrote:
On 07/08/2012 15:02, Neil Richards wrote:
:
From a Java Class Library point of view, it seems to me that the desire is to register shutdown hooks for any of these signals (HUP, INT and TERM) whose use has not been restricted by the VM.
So an attempt to register for each of these signals should be made, independent of the result of any of the other attempts.
To do anything else builds into the Java class library code assumptions about the behaviour of the VM which are inherently implementation (version?) specific, which is a brittle thing to do.
It currently assumes that the VM will either have restricted all these signals (the -Xrs case) or none of them.
This assumption holds true for the current version of Hotspot VM, but not necessarily for other VM implementations, whose mix of signal usage may differ.
So I think Frank's suggested change helps this code to adhere the VM / Class Library interface boundary, and so makes it less brittle.
As David said, the proposed change is harmless and I don't think anyone has any issue with it. It's really just us trying to understand whether there is really an issue here or not as it is has not been clear from the mails so far. I'm guessing it's AIX or J9 where -Xrs may be mapped to a different set of signals. FWIW, the termination setup has not been touched in>10 years. Looking at it now then it could have been done in other ways that wouldn't have been VM specific. Whether it's worth doing this now isn't clear as it just hasn't been an issue (to my knowledge anyway).
-Alan
Digging back into its history, I see that it all stems from running java under 'nohup' (e.g. 'nohup java ProgramWithShutdownHooks&').
'nohup' prevents things being registered against SIGHUP, so trying to do so causes an exception to be thrown.
When this happens, the Terminator code currently decides that it shouldn't bother trying to register shutdown hooks for SIGINT or SIGTERM either.
Thanks Neil that is a much more convincing justification.
The "interface is brittle" argument is rather tenous as the key issue is when the underlying method will throw IllegalArgumentException - which is inherently part of the unwritten contract with the native method and thence the VM. The code already assumes that it is okay to ignore the IllegalArgumentException, which implies it knows the exact circumstances when it will be thrown.
Anyway the change is okay to push. CR 7189865 created for this.
Thanks, David -----
The result is that shutdown hooks aren't currently run when a java process run using 'nohup' is sent a SIGINT or SIGTERM signal, because of the code's (false) assumption that a failure to register a handler for SIGHUP must mean that the VM is running in -Xrs mode.
I don't think the current observed behaviour in this scenario is either expected or desirable. It occurs for both J9 and Hotspot (at least, on my Ubuntu box, bash shell).
Hope this helps to clarify the scenario.
Regards, Neil
On 07/08/2012 19:07, Neil Richards wrote:
: Digging back into its history, I see that it all stems from running java under 'nohup' (e.g. 'nohup java ProgramWithShutdownHooks&').
'nohup' prevents things being registered against SIGHUP, so trying to do so causes an exception to be thrown.
When this happens, the Terminator code currently decides that it shouldn't bother trying to register shutdown hooks for SIGINT or SIGTERM either.
The result is that shutdown hooks aren't currently run when a java process run using 'nohup' is sent a SIGINT or SIGTERM signal, because of the code's (false) assumption that a failure to register a handler for SIGHUP must mean that the VM is running in -Xrs mode.
I don't think the current observed behaviour in this scenario is either expected or desirable. It occurs for both J9 and Hotspot (at least, on my Ubuntu box, bash shell).
Hope this helps to clarify the scenario.
Thanks for helping to clarify what this issue is about. One thing that still isn't clear (to me anyway) is whether IAE is really thrown in this case. I did a quick test on Ubuntu 12.04: $ nohup strace -f java Test 2> log & and in the log I see the sigactions as I expected: [pid 13829] rt_sigaction(SIGHUP, NULL, {SIG_IGN, [], 0}, 8) = 0 [pid 13829] rt_sigaction(SIGINT, NULL, {SIG_DFL, [], 0}, 8) = 0 [pid 13829] rt_sigaction(SIGTERM, NULL, {SIG_DFL, [], 0}, 8) = 0 So I don't think we are actually getting an IllegalArgumentException in this case. Maybe you are seeing something different? -Alan.
On Wed, 2012-08-08 at 13:28 +0100, Alan Bateman wrote:
On 07/08/2012 19:07, Neil Richards wrote:
: Digging back into its history, I see that it all stems from running java under 'nohup' (e.g. 'nohup java ProgramWithShutdownHooks&').
'nohup' prevents things being registered against SIGHUP, so trying to do so causes an exception to be thrown.
When this happens, the Terminator code currently decides that it shouldn't bother trying to register shutdown hooks for SIGINT or SIGTERM either.
The result is that shutdown hooks aren't currently run when a java process run using 'nohup' is sent a SIGINT or SIGTERM signal, because of the code's (false) assumption that a failure to register a handler for SIGHUP must mean that the VM is running in -Xrs mode.
I don't think the current observed behaviour in this scenario is either expected or desirable. It occurs for both J9 and Hotspot (at least, on my Ubuntu box, bash shell).
Hope this helps to clarify the scenario.
Thanks for helping to clarify what this issue is about. One thing that still isn't clear (to me anyway) is whether IAE is really thrown in this case. I did a quick test on Ubuntu 12.04:
$ nohup strace -f java Test 2> log &
and in the log I see the sigactions as I expected:
[pid 13829] rt_sigaction(SIGHUP, NULL, {SIG_IGN, [], 0}, 8) = 0 [pid 13829] rt_sigaction(SIGINT, NULL, {SIG_DFL, [], 0}, 8) = 0 [pid 13829] rt_sigaction(SIGTERM, NULL, {SIG_DFL, [], 0}, 8) = 0
So I don't think we are actually getting an IllegalArgumentException in this case. Maybe you are seeing something different?
-Alan.
Hi Alan, Apologies, I confused myself about the Hotspot behaviour. (I was testing with 'nohup', but not checking the contents of 'nohup.out' for the output from my registered shutdown hook - doh!). You're correct, when running with Hotspot with 'nohup', no exception is thrown when trying to register a handler for SIGHUP. When running with J9 with 'nohup', the VM notices that a handler registered for SIGHUP is never going to get triggered, so throws the exception to notify the caller (of Signal.handle( )) about this. Regards, Neil -- Unless stated above: IBM email: neil_richards at uk.ibm.com IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
On 08/08/2012 17:10, Neil Richards wrote:
: Hi Alan, Apologies, I confused myself about the Hotspot behaviour. (I was testing with 'nohup', but not checking the contents of 'nohup.out' for the output from my registered shutdown hook - doh!).
You're correct, when running with Hotspot with 'nohup', no exception is thrown when trying to register a handler for SIGHUP.
When running with J9 with 'nohup', the VM notices that a handler registered for SIGHUP is never going to get triggered, so throws the exception to notify the caller (of Signal.handle( )) about this.
Regards, Neil This probably comes back to documenting the interface between the VM and the libraries that you and Steve are working on. In this case, JVM_RegisterSignal is expected to return 1 when the action is SIG_IGN where it sounds like J9 is returning -1.
-Alan.
On Thu, 2012-08-09 at 09:54 +0100, Alan Bateman wrote:
On 08/08/2012 17:10, Neil Richards wrote:
: Hi Alan, Apologies, I confused myself about the Hotspot behaviour. (I was testing with 'nohup', but not checking the contents of 'nohup.out' for the output from my registered shutdown hook - doh!).
You're correct, when running with Hotspot with 'nohup', no exception is thrown when trying to register a handler for SIGHUP.
When running with J9 with 'nohup', the VM notices that a handler registered for SIGHUP is never going to get triggered, so throws the exception to notify the caller (of Signal.handle( )) about this.
Regards, Neil This probably comes back to documenting the interface between the VM and the libraries that you and Steve are working on. In this case, JVM_RegisterSignal is expected to return 1 when the action is SIG_IGN where it sounds like J9 is returning -1.
-Alan.
Hi Alan, Thanks for your response. First, I think the discussion on the particular VM implementation behaviour is a slight diversion from Frank's suggested change, which I believe is all about making the Java code more robust / agnostic to the VM implementation behaviour.
From the perspective of the code in Terminator.setup(), I think the pertinent question is: Should the code try to register shutdown hook handlers for other signals if the attempt to register for one signal fails with an exception? Or, to put it another way: Should the registration of the shutdown hook handler for each signal be independent of each other?
Frank's suggested change, and my argument, is that these registrations should be independent - i.e. that Terminator.setup() should register shutdown hook handlers for all (shutdown) signals that the system (VM) allows it to. It is self-evident from the Signal.handle() method that it may throw an exception on failure to register a handler - that's declared in its signature, after all - so Terminator.setup() should be coded to robustly handle that condition. Discussions on particular VM implementations' behaviours below Signal.handle() just serves to reinforce the rationale and efficacy of the suggested change, I feel. --- Having said that, and separate from the specifics of Frank's changes, I think it's interesting to consider what is the best behaviour of Signal.handle() (and JVM_RegisterSignal that underpins it). Let's consider the form of sun.misc.Signal.handle() (ignoring access modifiers for simplicity: SignalHandler handle(Signal sig, SignalHandler newHandler) throws IllegalArgumentException; So, it can return a SignalHandler object, or throw an IllegalArgumentException (IAG). If an exception is thrown, it is clear that the SignalHandler given to handle() has not been successfully registered. If, instead, a SignalHandler is returned, the returned value is generally that for the previously registered signal handler - i.e. the one that has just been replaced with the new handler. In J9's case, this is precisely the behaviour it provides: if the new handler is registered, the old handler is returned; if the new handler is not registered, an exception is thrown. However, in Hotspot's case, if the current handler is SIG_IGN, the new handler is not registered, no exception is thrown, and SIG_IGN is returned. So, on the surface, the call seems to have "succeeded", but actually, the new handler has not been registered. This behaviour forces a caller (who's interested in understanding if their new handler is actually getting registered) to understand this extra complexity in behaviour, and to do extra checking around the call to handle() in case it returns SIG_IGN to account for it. That doesn't seem to me like well-designed behaviour for the interface. I don't see any advantage gained in having this extra semantic complexity. Currently, both VMs decide not to replace SIG_IGN with any other handler, even if the SIG_IGN has been set up via a previous call to Signal.handle(). Thus setting a signal to be ignored is a one-way street to a dead end (in a vehicle with no reverse). But note that this behaviour is a choice of the VM (implementer). It isn't inherently the case that signal set to SIG_IGN can't be replaced with another action (at the C level), only that the JVM chooses never to do so. Perhaps in the future, Java might wish to allow the overriding of SIG_IGN signal handlers set up via a previous call to Signal.handle(), whilst respecting the setting if it has genuinely come from the OS / environment. (Also note this isn't only an issue for the HUP signal, either - using "trap '' SIGNUM" can set up (almost) any signal to be ignored.) So I'd submit J9's behaviour for JVM_RegisterSignal would be the better on which to standardize. --- However, I still consider that VM modification would be logically orthogonal to Frank's suggested change, and suggest that his change could continue to be approved for contribution at this point, regardless of the separate VM-related discussion in this area. Please get back to me with your thoughts and comments on any of the above. (If you agree to the separation of concerns I suggest, perhaps the discussion of the VM-related behaviour should be split to a separate thread ?) Regards, Neil -- Unless stated above: IBM email: neil_richards at uk.ibm.com IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
On 16/08/2012 16:32, Neil Richards wrote:
:
First, I think the discussion on the particular VM implementation behaviour is a slight diversion from Frank's suggested change, which I believe is all about making the Java code more robust / agnostic to the VM implementation behaviour. I think it was important to get some context as it was missing in the original mails.
From the perspective of the code in Terminator.setup(), I think the pertinent question is: Should the code try to register shutdown hook handlers for other signals if the attempt to register for one signal fails with an exception? Or, to put it another way: Should the registration of the shutdown hook handler for each signal be independent of each other?
Frank's suggested change, and my argument, is that these registrations should be independent - i.e. that Terminator.setup() should register shutdown hook handlers for all (shutdown) signals that the system (VM) allows it to.
I think we're in agreement on that. On JVM_RegisterSignal then it returns -1 if the signal is in use by the VM or there is an error examining/changing the action. If the signal is ignored (existing action is SIG_IGN) then it just returns SIG_IGN without attempting to change it. It probably should have defined a specific error for this case as the caller can't distinguish it from the case that the action was changed and the previous action was SIG_IGN. If there was an error for this case then sun.misc.Signal could have mapped it to an exception, perhaps IllegalStateException. Whether it's worth changing this now isn't clear to me but changing it is potentially disruptive. Disruptive meaning that it would change the behavior of sun.misc.Signal so that it would throw an exception for a case where it doesn't do so today. Folks shouldn't be using sun.misc.Signal of course but we know that they do.
:
However, I still consider that VM modification would be logically orthogonal to Frank's suggested change, and suggest that his change could continue to be approved for contribution at this point, regardless of the separate VM-related discussion in this area.
Both David and I have already reviewed Frank's change and okay with it. I'll leave it to you whether you want to propose a change to JVM_RegisterSignal. -Alan
Hi Alan and David, So is it OK to commit the patch in Terminator class? I think Neil will continue proposing the change in cvmi mailing list. Many thanks to all. Best regards, Frank On 8/17/2012 4:58 PM, Alan Bateman wrote:
On 16/08/2012 16:32, Neil Richards wrote:
:
However, I still consider that VM modification would be logically orthogonal to Frank's suggested change, and suggest that his change could continue to be approved for contribution at this point, regardless of the separate VM-related discussion in this area.
Both David and I have already reviewed Frank's change and okay with it.
I'll leave it to you whether you want to propose a change to JVM_RegisterSignal.
-Alan
On 22/08/2012 12:11 PM, Frank Ding wrote:
Hi Alan and David, So is it OK to commit the patch in Terminator class? I think Neil will continue proposing the change in cvmi mailing list.
As Alan already said: "Both David and I have already reviewed Frank's change and okay with it." Cheers, David
Many thanks to all.
Best regards, Frank
On 8/17/2012 4:58 PM, Alan Bateman wrote:
On 16/08/2012 16:32, Neil Richards wrote:
:
However, I still consider that VM modification would be logically orthogonal to Frank's suggested change, and suggest that his change could continue to be approved for contribution at this point, regardless of the separate VM-related discussion in this area.
Both David and I have already reviewed Frank's change and okay with it.
I'll leave it to you whether you want to propose a change to JVM_RegisterSignal.
-Alan
Thanks all. I created sun bug 7193463 to track the code change. Best regards, Frank On 8/22/2012 10:13 AM, David Holmes wrote:
On 22/08/2012 12:11 PM, Frank Ding wrote:
Hi Alan and David, So is it OK to commit the patch in Terminator class? I think Neil will continue proposing the change in cvmi mailing list.
As Alan already said:
"Both David and I have already reviewed Frank's change and okay with it."
Cheers, David
Many thanks to all.
Best regards, Frank
On 8/17/2012 4:58 PM, Alan Bateman wrote:
On 16/08/2012 16:32, Neil Richards wrote:
:
However, I still consider that VM modification would be logically orthogonal to Frank's suggested change, and suggest that his change could continue to be approved for contribution at this point, regardless of the separate VM-related discussion in this area.
Both David and I have already reviewed Frank's change and okay with it.
I'll leave it to you whether you want to propose a change to JVM_RegisterSignal.
-Alan
On 23/08/2012 6:05 PM, Frank Ding wrote:
Thanks all. I created sun bug 7193463 to track the code change.
We already have 7189865 for this. David
Best regards, Frank
On 8/22/2012 10:13 AM, David Holmes wrote:
On 22/08/2012 12:11 PM, Frank Ding wrote:
Hi Alan and David, So is it OK to commit the patch in Terminator class? I think Neil will continue proposing the change in cvmi mailing list.
As Alan already said:
"Both David and I have already reviewed Frank's change and okay with it."
Cheers, David
Many thanks to all.
Best regards, Frank
On 8/17/2012 4:58 PM, Alan Bateman wrote:
On 16/08/2012 16:32, Neil Richards wrote:
:
However, I still consider that VM modification would be logically orthogonal to Frank's suggested change, and suggest that his change could continue to be approved for contribution at this point, regardless of the separate VM-related discussion in this area.
Both David and I have already reviewed Frank's change and okay with it.
I'll leave it to you whether you want to propose a change to JVM_RegisterSignal.
-Alan
Hi David, Sorry that Jonathan has already committed the code with bug id 7193463. Could you please dup the 2 bugs? Best regards, Frank On 8/23/2012 4:27 PM, David Holmes wrote:
On 23/08/2012 6:05 PM, Frank Ding wrote:
Thanks all. I created sun bug 7193463 to track the code change.
We already have 7189865 for this.
David
Best regards, Frank
On 8/22/2012 10:13 AM, David Holmes wrote:
On 22/08/2012 12:11 PM, Frank Ding wrote:
Hi Alan and David, So is it OK to commit the patch in Terminator class? I think Neil will continue proposing the change in cvmi mailing list.
As Alan already said:
"Both David and I have already reviewed Frank's change and okay with it."
Cheers, David
Many thanks to all.
Best regards, Frank
On 8/17/2012 4:58 PM, Alan Bateman wrote:
On 16/08/2012 16:32, Neil Richards wrote:
:
However, I still consider that VM modification would be logically orthogonal to Frank's suggested change, and suggest that his change could continue to be approved for contribution at this point, regardless of the separate VM-related discussion in this area.
Both David and I have already reviewed Frank's change and okay with it.
I'll leave it to you whether you want to propose a change to JVM_RegisterSignal.
-Alan
On 23/08/2012 6:37 PM, Frank Ding wrote:
Hi David, Sorry that Jonathan has already committed the code with bug id 7193463. Could you please dup the 2 bugs?
Alan has already taken care of it. David
Best regards, Frank
On 8/23/2012 4:27 PM, David Holmes wrote:
On 23/08/2012 6:05 PM, Frank Ding wrote:
Thanks all. I created sun bug 7193463 to track the code change.
We already have 7189865 for this.
David
Best regards, Frank
On 8/22/2012 10:13 AM, David Holmes wrote:
On 22/08/2012 12:11 PM, Frank Ding wrote:
Hi Alan and David, So is it OK to commit the patch in Terminator class? I think Neil will continue proposing the change in cvmi mailing list.
As Alan already said:
"Both David and I have already reviewed Frank's change and okay with it."
Cheers, David
Many thanks to all.
Best regards, Frank
On 8/17/2012 4:58 PM, Alan Bateman wrote:
On 16/08/2012 16:32, Neil Richards wrote: > : > > However, I still consider that VM modification would be logically > orthogonal to Frank's suggested change, and suggest that his change > could continue to be approved for contribution at this point, > regardless > of the separate VM-related discussion in this area. > Both David and I have already reviewed Frank's change and okay with it.
I'll leave it to you whether you want to propose a change to JVM_RegisterSignal.
-Alan
participants (4)
-
Alan Bateman
-
David Holmes
-
Frank Ding
-
Neil Richards