RFR JDK-8225474: JDI connector accept fails "Address already in use" with concurrent listeners
Andrew Leonard
andrew_m_leonard at uk.ibm.com
Mon Jul 1 19:41:29 UTC 2019
Any one able to review please?
Thanks
Andrew
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leonard at uk.ibm.com
From: Andrew Leonard/UK/IBM
To: Alan Bateman <Alan.Bateman at oracle.com>
Cc: serviceability-dev at 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_leonard at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190701/001afd73/attachment-0001.html>
More information about the serviceability-dev
mailing list