Hi Alan,
So I have done some research and analysis, and I have come to some
conclusions over the key stop/start/accept methods in
GenericListeningConnector:
stopListening() {
(1) TransportService.ListenKey listener = listenMap.get(args);
if (listener == null) {
throw new IllegalConnectorArgumentsException("Not listening",
new ArrayList<>(args.keySet()));
}
transportService.stopListening(listener);
(2) listenMap.remove(args);
}
As you state the listenMap.get() should be a straight remove(), hence this
method should be changed to this:
stopListening() {
(1) TransportService.ListenKey listener = listenMap.remove(args);
if (listener == null) {
throw new IllegalConnectorArgumentsException("Not listening",
new ArrayList<>(args.keySet()));
}
transportService.stopListening(listener);
}
startListening() {
(1) TransportService.ListenKey listener = listenMap.get(args);
| if (listener != null) {
| throw new IllegalConnectorArgumentsException("Already
listening",
| new ArrayList<>(args.keySet()));
V }
(2) listener = transportService.startListening(address);
(3) updateArgumentMapIfRequired(args, listener);
(4) listenMap.put(args, listener);
}
(1) This listenMap(get) check and throw is actually purely a simplified
upfront check to see if it is already listening, the
transportService.startListening() (2) ulitmately will fail if it is
already. However, strictly speaking another thread could be in the throws
of stopping, where it "removes" the map entry then calls
ts.stopListening() (in the updated method), so as it stands this check
could pass but ts.startListening() fails as the other thread has not
stopped yet...but I actually think that is ok. Ultimately the thread has
asked to listen on a port that hasn't finished being used yet.
(3) updates the "args" -> "port" in the sub-class for the WildCard
listening so that the map entry put (4) is unique to what port is actually
listening.
The accept() method does not update the map, and I cannot see anything
wrong from it's current implementation.
So apart from the update to stopListening() and the fix to use
ConcurrentHashMap I think it seems fine. The issue with the "bug" was
corruption of the HashMap so that the accept() method incorrectly thought
the port was not being listened upon and then tried to do another
startListening().
Thoughts?
Thanks
Andrew
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com
From: Alan Bateman <alan.bate...@oracle.com>
To: "serguei.spit...@oracle.com" <serguei.spit...@oracle.com>, Andrew
Leonard <andrew_m_leon...@uk.ibm.com>, serviceability-dev@openjdk.java.net
Date: 08/06/2019 10:12
Subject: Re: RFR JDK-8225474: JDI connector accept fails "Address
already in use" with concurrent listeners
On 08/06/2019 05:53, serguei.spit...@oracle.com wrote:
> Hi Andrew,
>
> It looks good to me.
> Thank you for your investigation and taking care about this!
I think this one needs further analysis as changing listenMap to be a
CHM does not make this connector thread safe and doesn't address a
number of other places where the map is accessed without
synchronization. For example, startListening would need to be changed to
use `putIfAbsent`, stopListening would need to use `remove` rather than
get + remote. There are several others once you start looking at the
sub-classes, e.g. SocketListeningConnector.updateArgumentMapIfRequired.
At the API level, JDI does not require connectors to be thread safe. I
agree it is desirable for some of the connector implementations to be
thread safe, particularly ListeningConnectors as an obvious usage will
be for a listener thread to hand off a VirtualMachine to another thread.
Andrew - do you have cycles to investigate this further? I'm not opposed
to changing the map to be a CHM but I think it needs further work.
-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
