Re: RFR JDK-8225474: JDI connector accept fails "Address already in use" with concurrent listeners

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8225474: JDI connector accept fails "Address already in use" with concurrent listeners

Andrew Leonard
Any one able to review please?
Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: [hidden email]





From:        Andrew Leonard/UK/IBM
To:        Alan Bateman <[hidden email]>
Cc:        [hidden email]
Date:        19/06/2019 18:29
Subject:        Re: RFR JDK-8225474: JDI connector accept fails "Address already in use" with concurrent listeners



Alan and other reviewers please?,
Firstly apologies for the long post, but I wanted to be thorough.

Following previous comments I have reviewed my understanding of concurrency and "safe publishing" in the JMM, and I have a fairly clear understanding now. I have reworked my fix to the issue associated with concurrent JDI connector listeners after thoroughly reviewing and walking through the sun.tools.jdi Connector implementation and also reading the spec javadoc.
My Goal: To make JDI Connector and associated listening/attaching APIs "thread-safe", for the purpose of supporting a debugger use case where multiple listening/attaching "threads" are used.

From the review I determined the following key components are already thread-safe:
- VirtualMachineImpl
- VirtualMachineManagerImpl

The components that needed some work were:
- ConnectorImpl and all its sub-classes
- A Minor fix to SocketTransportService

My new patch is available for review here: http://cr.openjdk.java.net/~aleonard/8225474/webrev.05/

Tests run successfully:
- new JdwpConcurrentAttachTest.java which performs a multi-threaded stress test of the start/stopListening process where the "bug" occurs
- jtreg -jdk:$PRODUCT_HOME -concurrency:12 -v1 -timeoutfactor:20 langtools/jdk/jshell
- jtreg -jdk:$PRODUCT_HOME -timeoutfactor:20 -v1 jdk/com/sun/jdi  (2 tests failed that failed before for unrelated reasons)

The following summarizes the key changes and why:

ConnectorImpl:
    final Map<String, Argument> defaultArguments = new LinkedHashMap<>();
        ==> Made "final" so that Connector object defaultArguments is safely published to other threads after construction

    defaultArguments():
+   synchronized(defaultArguments) {
        Collection<Argument> values = defaultArguments.values();
    ==> Synchronize on defaultArguments before iterating over values to return a "copy" to debugger

    addString/Boolean/Integer/SelectedArgument:
+        synchronized(defaultArguments) {
             defaultArguments.put(name,
    ==> synchronize on defaultArguments for updates

    toString:
+        synchronized(defaultArguments) {
            Iterator<Argument> iter = defaultArguments().values().iterator();
    ==> synchronize on defaultArguments prior to values iteration, and to create a "happens-before" relationship

+   synchronized(this) {
            if (messages == null) {
                messages = ResourceBundle.getBundle("com.sun.tools.jdi.resources.jdi");
            }
+   }
    ==> Protect messages construction

    ArgumentImpl
        private final String name;
        private final String label;
        private final String description;
        private volatile String value;
        private final boolean mustSpecify;
        ==> final all except value which is volatile, so that Argument can be safely published to other threads after construction
        ==> value is "volatile" as this can be set by debuggers, so must set volatile to ensure if passed to other thread a "happens-before" is ensured

        BooleanArgumentImpl
+            synchronized(ConnectorImpl.class) {
                if(trueString == null) {
                    trueString = getString("true");
                    falseString = getString("false");
                }
+            }
    ==> synchronized on ConnectorImpl.class object to ensure lock for initializing the static fields


GenericListeningConnector:        
    final Map<Map<String,? extends Connector.Argument>, TransportService.ListenKey>  listenMap;
    final TransportService transportService;
    final Transport transport;
        ==> "final" these to ensure "safe publication" to other threads after Connector construction

       
    private GenericListeningConnector(TransportService ts,
                                       boolean addAddressArgument,
+                                      Transport tsp)
         ==> Added Transport param to constructor so that sub-classes can pass in their desired Transport and then allow transport to be "final" for "safe publication". Previously transport was being constructed "twice" wastefully.
         
    listenMap = new ConcurrentHashMap<Map<String, ? extends Connector.Argument>, TransportService.ListenKey>(10);
        ==> Make listenMap a ConcurrentHashMap so that it is "thread-safe" for access/updates

       
    public synchronized String startListening/stopListening(..
        ==> Made start & stop listening "synchronized" so that management of listenMap entry and transportService state are locked&synchronized

       
GenericAttachingConnector:
    final TransportService transportService;
    final Transport transport;
        ==> Made "final" so that Connector can be safely published

     private GenericAttachingConnector(TransportService ts,
                                       boolean addAddressArgument,
+                                      Transport tsp)
        ==> Added Transport param to constructor so that sub-classes can pass in their desired Transport and then allow transport to be "final" for "safe publication". Previously transport was being constructed "twice" wastefully.

ProcessAttachingConnector:        
-    com.sun.tools.attach.VirtualMachine vm;
     final Transport transport;
        ==> Removed "unused" vm field
        ==> Made transport final to allow "safe publication"

     public Transport transport() {
-        if (transport == null) {
-            return new Transport() {
-                public String name() {
-                    return "local";
-                }
-            };
-        }
         return transport;
        ==> Removed creation by this method, as transport is always constructed in constructor and is now final. This now matches transport() method in all the other connector impls

SocketAttaching/ListeningConnector:
     public SocketAttaching/ListeningConnector() {
-        super(new SocketTransportService());
+        super(new SocketTransportService(), new Transport() {
+                                              public String name() {
+                                                  return "dt_socket";  // for compatibility reasons
+                                              }
+                                            });
        ==> Construct Transport() during super constructor where transport is now "final" to ensure "safe publication", and avoids wasteful double construction that was happening before.

SocketTransportService:
     static class SocketListenKey extends ListenKey {
         final ServerSocket ss;
        ==> Made "final" so that ListenKey returned by startListening() is safe for publication, eg.if one thread starts listening and passes to another thread to accept connection...







Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: [hidden email]


Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU



Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8225474: JDI connector accept fails "Address already in use" with concurrent listeners

Alan Bateman
On 01/07/2019 20:41, Andrew Leonard wrote:
Any one able to review please?
This one will take a significant time to review. We also need to figure out if an @implNote if needed as nobody using these connectors will know (from the javadoc) that some of the implementations are thread safe. I hope to get to it in the next few weeks.

-Alan.
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8225474: JDI connector accept fails "Address already in use" with concurrent listeners

Andrew Leonard
Thanks Alan, much appreciated.

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: [hidden email]





From:        Alan Bateman <[hidden email]>
To:        Andrew Leonard <[hidden email]>
Cc:        [hidden email]
Date:        02/07/2019 07:45
Subject:        Re: RFR JDK-8225474: JDI connector accept fails "Address already in use" with concurrent listeners




On 01/07/2019 20:41, Andrew Leonard wrote:
Any one able to review please?
This one will take a significant time to review. We also need to figure out if an @implNote if needed as nobody using these connectors will know (from the javadoc) that some of the implementations are thread safe. I hope to get to it in the next few weeks.

-Alan.



Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU