[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR JDK-8225474: JDI connector accept fails "Address already in use" with concurrent listeners
From: Andrew Leonard <andrew_m_leonard () uk ! ibm ! com>
Date: 2019-06-19 17:29:08
Message-ID: OF80183130.611AD6BA-ON0025841E.004F3EAC-8025841E.00600CED () notes ! na ! collabserv ! com
[Download RAW message or body]
This is a multipart message in MIME format.
--=_alternative 00600C648025841E_=
Content-Type: text/plain; charset="US-ASCII"
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@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
--=_alternative 00600C648025841E_=
Content-Type: text/html; charset="US-ASCII"
<span style=" font-size:10pt;font-family:sans-serif">Alan and other reviewers
please?,</span>
<br><span style=" font-size:10pt;font-family:sans-serif">Firstly apologies
for the long post, but I wanted to be thorough.</span>
<br>
<br><span style=" font-size:10pt;font-family:sans-serif">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.</span>
<br><span style=" font-size:10pt;font-family:sans-serif">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.</span>
<br>
<br><span style=" font-size:10pt;font-family:sans-serif">From the review
I determined the following key components are already thread-safe:</span>
<br><span style=" font-size:10pt;font-family:sans-serif">- VirtualMachineImpl</span>
<br><span style=" font-size:10pt;font-family:sans-serif">- \
VirtualMachineManagerImpl</span> <br>
<br><span style=" font-size:10pt;font-family:sans-serif">The components
that needed some work were:</span>
<br><span style=" font-size:10pt;font-family:sans-serif">- ConnectorImpl
and all its sub-classes</span>
<br><span style=" font-size:10pt;font-family:sans-serif">- A Minor fix
to SocketTransportService</span>
<br>
<br><span style=" font-size:10pt;font-family:sans-serif">My new patch is
available for review here: </span><a \
href=http://cr.openjdk.java.net/~aleonard/8225474/webrev.05/><span style=" \
font-size:10pt;color:blue;font-family:sans-serif">http://cr.openjdk.java.net/~aleonard/8225474/webrev.05/</span></a>
<br>
<br><span style=" font-size:10pt;font-family:sans-serif">Tests run \
successfully:</span> <br><span style=" font-size:10pt;font-family:sans-serif">- new \
JdwpConcurrentAttachTest.java which performs a multi-threaded stress test of the \
start/stopListening process where the "bug" occurs</span>
<br><span style=" font-size:10pt;font-family:sans-serif">- jtreg -jdk:$PRODUCT_HOME
-concurrency:12 -v1 -timeoutfactor:20 langtools/jdk/jshell</span>
<br><span style=" font-size:10pt;font-family:sans-serif">- jtreg -jdk:$PRODUCT_HOME
-timeoutfactor:20 -v1 jdk/com/sun/jdi (2 tests failed that failed
before for unrelated reasons)</span>
<br>
<br><span style=" font-size:10pt;font-family:sans-serif">The following
summarizes the key changes and why:</span>
<br>
<br><span style=" font-size:10pt;font-family:sans-serif">ConnectorImpl:</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
final Map<String, Argument> defaultArguments = new \
LinkedHashMap<>();</span> <br><span style=" \
font-size:10pt;font-family:sans-serif"> ==> Made \
"final" so that Connector object defaultArguments is safely published to \
other threads after construction</span> <br>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
defaultArguments():</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">+
synchronized(defaultArguments) {</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
Collection<Argument> values = \
defaultArguments.values();</span> <br><span style=" \
font-size:11pt;color:#00c200;font-family:Menlo-Regular"> ==> \
Synchronize on defaultArguments before iterating over values to return a \
"copy" to debugger</span> <br>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
addString/Boolean/Integer/SelectedArgument:</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">+
synchronized(defaultArguments) {</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
defaultArguments.put(name,</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
==> synchronize on defaultArguments for updates</span>
<br>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
toString:</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">+
synchronized(defaultArguments) {</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
Iterator<Argument> iter = \
defaultArguments().values().iterator();</span> <br><span style=" \
font-size:11pt;color:#00c200;font-family:Menlo-Regular"> ==> \
synchronize on defaultArguments prior to values iteration, and to create a \
"happens-before" relationship</span> <br>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">+
synchronized(this) {</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
if (messages == null) {</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
messages = \
ResourceBundle.getBundle("com.sun.tools.jdi.resources.jdi");</span> \
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular"> \
}</span> <br><span style=" \
font-size:11pt;font-family:Menlo-Regular">+ }</span> <br><span style=" \
font-size:11pt;font-family:Menlo-Regular"> ==> Protect messages \
construction</span> <br>
<br><span style=" font-size:11pt;font-family:Menlo-Regular">
ArgumentImpl</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
private final String name;</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
private final String label;</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
private final String description;</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
private volatile String value;</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
private final boolean mustSpecify;</span>
<br><span style=" font-size:10pt;font-family:sans-serif">
==> final all except value which is volatile, so that
Argument can be safely published to other threads after construction</span>
<br><span style=" font-size:10pt;font-family:sans-serif">
==> 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</span>
<br>
<br><span style=" font-size:10pt;font-family:sans-serif">
BooleanArgumentImpl</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">+
synchronized(ConnectorImpl.class)
{</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
if(trueString == null)
{</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
trueString
= getString("true");</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
falseString
= getString("false");</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
}</span>
<br><span style=" font-size:11pt;font-family:Menlo-Regular">+
}</span>
<br><span style=" font-size:11pt;font-family:Menlo-Regular">
==> synchronized on ConnectorImpl.class object to ensure lock for initializing
the static fields</span>
<br>
<br>
<br><span style=" font-size:10pt;font-family:sans-serif">GenericListeningConnector:
</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
final Map<Map<String,? extends Connector.Argument>, \
TransportService.ListenKey> listenMap;</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
final TransportService transportService;</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
final Transport transport;</span>
<br><span style=" font-size:10pt;font-family:sans-serif">
==> "final" these to ensure "safe publication"
to other threads after Connector construction</span>
<br>
<br><span style=" font-size:10pt;font-family:sans-serif">
</span>
<br><span style=" font-size:11pt;font-family:Menlo-Regular">
private GenericListeningConnector(TransportService ts,</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
boolean \
addAddressArgument,</span> <br><span style=" \
font-size:11pt;color:#00c200;font-family:Menlo-Regular">+ \
\
Transport tsp)</span> <br><span style=" \
font-size:10pt;font-family:sans-serif"> ==> 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.</span> <br><span style=" font-size:10pt;font-family:sans-serif"> \
</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
listenMap = new ConcurrentHashMap<Map<String, ? extends \
Connector.Argument>, TransportService.ListenKey>(10);</span>
<br><span style=" font-size:10pt;font-family:sans-serif">
==> Make listenMap a ConcurrentHashMap so that it is \
"thread-safe" for access/updates</span>
<br>
<br><span style=" font-size:10pt;font-family:sans-serif">
</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
public synchronized String startListening/stopListening(..</span>
<br><span style=" font-size:10pt;font-family:sans-serif">
==> Made start & stop listening "synchronized"
so that management of listenMap entry and transportService state are \
locked&synchronized</span> <br>
<br><span style=" font-size:10pt;font-family:sans-serif">
</span>
<br><span style=" font-size:10pt;font-family:sans-serif">GenericAttachingConnector:</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
final TransportService transportService;</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
final Transport transport;</span>
<br><span style=" font-size:10pt;font-family:sans-serif">
==> Made "final" so that Connector can be safely
published</span>
<br>
<br><span style=" font-size:11pt;font-family:Menlo-Regular">
private GenericAttachingConnector(TransportService ts,</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
boolean \
addAddressArgument,</span> <br><span style=" \
font-size:11pt;color:#00c200;font-family:Menlo-Regular">+ \
\
Transport tsp)</span> <br><span style=" \
font-size:10pt;font-family:sans-serif"> ==> 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.</span> <br>
<br><span style=" font-size:10pt;font-family:sans-serif">ProcessAttachingConnector:
</span>
<br><span style=" font-size:11pt;color:#a11f12;font-family:Menlo-Regular">-
com.sun.tools.attach.VirtualMachine vm;</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
final Transport transport;</span>
<br><span style=" font-size:10pt;font-family:sans-serif">
==> Removed "unused" vm field</span>
<br><span style=" font-size:10pt;font-family:sans-serif">
==> Made transport final to allow "safe \
publication"</span> <br>
<br><span style=" font-size:11pt;font-family:Menlo-Regular">
public Transport transport() {</span>
<br><span style=" font-size:11pt;color:#a11f12;font-family:Menlo-Regular">-
if (transport == null) {</span>
<br><span style=" font-size:11pt;color:#a11f12;font-family:Menlo-Regular">-
return new Transport() {</span>
<br><span style=" font-size:11pt;color:#a11f12;font-family:Menlo-Regular">-
public String name()
{</span>
<br><span style=" font-size:11pt;color:#a11f12;font-family:Menlo-Regular">-
return
"local";</span>
<br><span style=" font-size:11pt;color:#a11f12;font-family:Menlo-Regular">-
}</span>
<br><span style=" font-size:11pt;color:#a11f12;font-family:Menlo-Regular">-
};</span>
<br><span style=" font-size:11pt;color:#a11f12;font-family:Menlo-Regular">-
}</span>
<br><span style=" font-size:11pt;font-family:Menlo-Regular">
return transport;</span>
<br><span style=" font-size:10pt;font-family:sans-serif">
==> 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</span>
<br>
<br><span style=" font-size:10pt;font-family:sans-serif">SocketAttaching/ListeningConnector:</span>
<br><span style=" font-size:11pt;font-family:Menlo-Regular">
public SocketAttaching/ListeningConnector() {</span>
<br><span style=" font-size:11pt;color:#a11f12;font-family:Menlo-Regular">-
super(new SocketTransportService());</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">+
super(new SocketTransportService(), new Transport()
{</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">+
public String name() {</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">+
return "dt_socket"; // for compatibility
reasons</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">+
}</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">+
\
});</span> <br><span style=" font-size:10pt;font-family:sans-serif"> \
==> Construct Transport() during super constructor where
transport is now "final" to ensure "safe publication",
and avoids wasteful double construction that was happening before.</span>
<br>
<br><span style=" font-size:10pt;font-family:sans-serif">SocketTransportService:</span>
<br><span style=" font-size:11pt;font-family:Menlo-Regular">
static class SocketListenKey extends ListenKey {</span>
<br><span style=" font-size:11pt;color:#00c200;font-family:Menlo-Regular">
final ServerSocket ss;</span>
<br><span style=" font-size:10pt;font-family:sans-serif">
==> 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...</span>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br><span style=" font-size:12pt">Andrew Leonard<br>
Java Runtimes Development<br>
IBM Hursley<br>
IBM United Kingdom Ltd<br>
internet email: andrew_m_leonard@uk.ibm.com </span>
<br><span style=" font-size:10pt;font-family:sans-serif"><br>
Unless stated otherwise above:<br>
IBM United Kingdom Limited - Registered in England and Wales with number
741598. <br>
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6
3AU<br>
</span>
--=_alternative 00600C648025841E_=--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic