Any one able to review please?
Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 




From:   Andrew Leonard/UK/IBM
To:     Alan Bateman <alan.bate...@oracle.com>
Cc:     serviceability-dev@openjdk.java.net
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: andrew_m_leon...@uk.ibm.com 

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 via email to