Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

8288140: Avoid redundant Hashtable.get call in Signal.handle #9100

Closed
wants to merge 3 commits into from
Closed

8288140: Avoid redundant Hashtable.get call in Signal.handle #9100

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec32c3f
[PATCH] Avoid redundant Hashtable.get call in Signal.handle
turbanoff Jun 9, 2022
220d3b6
[PATCH] Avoid redundant Hashtable.get call in Signal.handle
turbanoff Jun 9, 2022
829f9e2
8288140: Avoid redundant Hashtable.get call in Signal.handle
turbanoff Jun 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/java.base/share/classes/jdk/internal/misc/Signal.java
View file
Open in desktop
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1998, 2016, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1998, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -174,8 +174,7 @@ public static synchronized Signal.Handler handle(Signal sig,
}
signals.put(sig.number, sig);
synchronized (handlers) {
Signal.Handler oldHandler = handlers.get(sig);
handlers.remove(sig);
Signal.Handler oldHandler = handlers.remove(sig);
if (newH == 2) {
handlers.put(sig, handler);
}
Copy link
Contributor

@dmlloyd dmlloyd Jun 9, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you made this change instead:

Suggested change
Signal.Handler oldHandler = handlers.remove(sig);
if (newH == 2) {
handlers.put(sig, handler);
}
Signal.Handler oldHandler;
if (newH == 2) {
oldHandler = handlers.put(sig, handler);
} else {
oldHandler = handlers.remove(sig);
}

Wouldn't you be able to remove the entire synchronized block?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello David, I suspect you mean handlers.put(sig, handler) and not handlers.replace(...)? And yes, I think what you suggest will help remove the synchronized block here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, yes you are correct!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
I think the synchronized block was redundant already in original code. Since the entire handle method is static synchronized and it is the only method that modifies the handlers and signals maps.
But even with so much redundant synchronization, the Signal class is not without races. There are multiple possible races in dispatch(int number) method which is called from native code to dispatch a signal:

  • race no. 1: dispatch method (not synchronized) performs 2 independent lookups into signals and handlers maps respectively, assuming their inter-referenced state is consistent. But handle method may be concurrently modifying them, so dispatch may see updated state of signals map while seeing old state of handlers map or vice versa.
  • race no. 2: besides signals and handlers there is a 3rd map in native code, updated with handle0 native method. Native code dispatches signals according to that native map, forwarding them to either native handlers or to dispatch Java method. But handle method may be modifying that native map concurrently, so dispatch may be called as a consequence of updated native map while seeing old states of signals and handlers maps.

I'm sure I might have missed some additional races.

How to fix this? Is it even necessary? I think that this internal API is not used frequently so this was hardly an issue. But anyway, it would be a challenge. While the two java maps: handlers and signals could be replaced with a single map, there is the 3rd map in native code. We would not want to make dispatch method synchronized, so with careful ordering of modifications it is perhaps possible to account for races and make them harmless...
WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
            }
        }
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down