RFR: 8288140: Avoid redundant Hashtable.get call in Signal.handle [v2]

[Andrey Turbanov]

On Tue, 14 Jun 2022 19:20:52 GMT, Andrey Turbanov <aturbanov at openjdk.org> wrote:

>> Well, studying this further, I think some races are already accounted for and are harmless except one. The `handle` method 1st attempts to register handler in native code via call to `handle0` and then updates the `signals` map.. As soon as native code registers the handler, `dispatch` may be called and the lookup into `signals` map may return null which would cause NPE in the following lookup into `handlers` map. So there are two possibilities to fix this:
>> - guard for null result from `singnals` lookup, or
>> - swap the order of modifying the `signals` map and a call to `handle0` native method. You could even update the `signals` map with `.putIfAbsent()` to avoid multiple reassignment with otherwise equal instances. This may unnecessarily establish a mapping for a Signal the can later not be registered, so you can remove it from the `signals` map in that case to clean-up.
>> The possible case when the lookup into `handlers` map returns null Handler is already taken into account, but there is a possible case where a NativeHandler is replaced with a java Handler implementation and `dispatch` is therefore already called, but `handlers` map lookup still returns a NativeHandler. In that case the NativeHandler::handle method will throw exception. To avoid that, NativeHandler::handle could be an empty method instead.
>
> I tried to reproduce this possible problem via jcstress, but never caught this race.
> 
> package org.openjdk.jcstress.samples.jmm.basic;
> 
> import org.openjdk.jcstress.annotations.*;
> import org.openjdk.jcstress.infra.results.ZZZZZZZZ_Result;
> import sun.misc.Signal;
> import sun.misc.SignalHandler;
> 
> import java.io.IOException;
> 
> import static org.openjdk.jcstress.annotations.Expect.*;
> 
> @JCStressTest
> @Outcome(id = "false, false, false, false, false, false, false, false", expect = ACCEPTABLE, desc = "All results are ok")
> @Outcome(id = ".*", expect = ACCEPTABLE_INTERESTING, desc = "All results are ok")
> @State
> public class BasicJMM_11_Signal {
> 
>     /*
>         How to run this test:
>             $ /home/turbanoff/.jdks/liberica-18.0.1.1/bin/java -jar jcstress-samples/target/jcstress.jar -t BasicJMM_11_Signal -v
>             $ /home/turbanoff/Projects/official_jdk/build/linux-x86_64-server-fastdebug/jdk/bin/java -jar jcstress-samples/target/jcstress.jar -t BasicJMM_11_Signal -v
>             $ /home/turbanoff/Projects/jdk2/build/linux-x86_64-server-release/jdk/bin/java -jar jcstress-samples/target/jcstress.jar -t BasicJMM_11_Signal -v
>      */
> 
>     private static final Signal[] signals = new Signal[]{
>             new Signal("URG"),
> //            new Signal("USR2"),
> //            new Signal("ALRM"),
> //            new Signal("USR1"),
>             new Signal("CHLD"),
>             new Signal("XFSZ"),
>             new Signal("CONT"),
> //            new Signal("POLL"),
>             new Signal("WINCH"),
> //            new Signal("IO"),
>             new Signal("PIPE"),
> //            new Signal("HUP"),
> //            new Signal("POLL"),
> //            new Signal("PROF"),
> //            new Signal("INT"),
> //            new Signal("STKFLT"),
>             new Signal("TSTP"),
> //            new Signal("SYS"),
> //            new Signal("TERM"),
> //            new Signal("TRAP"),
> //            new Signal("ABRT"),
>             new Signal("TTOU"),
>             new Signal("TTIN"),
> //            new Signal("BUS"),
> //            new Signal("VTALRM"),
> //            new Signal("XCPU"),
> //            new Signal("PWR")
>     };
> 
>     private static final String[][] commands = new String[signals.length][];
>     private static final long pid = ProcessHandle.current().pid();
> 
> 
>     static {
>         for (int i = 0; i < commands.length; i++) {
>             commands[i] = new String[]{ "kill", "-" + signals[i].getName(), Long.toString(pid) };
>         }
>     }
> 
>     @Actor
>     public void thread1() {
>         for (String[] command : commands) {
>             try {
>                 new ProcessBuilder(command)
>                         .directory(null)
>                         .start();
>             } catch (IOException e) {
>                 e.printStackTrace();
>             }
>         }
>     }
> 
>     @Actor
>     public void thread2(ZZZZZZZZ_Result r) {
>         for (int i = 0; i < signals.length; i++) {
>             Signal signal = signals[i];
>             Signal.handle(signal, new MySignalHandler(i, r));
>         }
>     }
> 
>     private static class MySignalHandler implements SignalHandler {
>         private final int num;
>         private final ZZZZZZZZ_Result r;
> 
>         public MySignalHandler(int num, ZZZZZZZZ_Result r) {
>             this.num = num;
>             this.r = r;
>         }
> 
>         @Override
>         public void handle(Signal sig) {
>             switch (num) {
>                 case 0:
>                     r.r1 = true;
>                     break;
>                 case 1:
>                     r.r2 = true;
>                     break;
>                 case 2:
>                     r.r3 = true;
>                     break;
>                 case 3:
>                     r.r4 = true;
>                 case 4:
>                     r.r5 = true;
>                     break;
>                 case 5:
>                     r.r6 = true;
>                     break;
>                 case 6:
>                     r.r7 = true;
>                     break;
>                 case 7:
>                     r.r8 = true;
>                     break;
>             }
>         }
>     }
> }

I believe fixing this race is out of scope of current PR. Feel free to open a separate issue :)

-------------

PR: https://git.openjdk.org/jdk/pull/9100