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
