RFR JDK-8225474: JDI connector accept fails "Address already in use" with concurrent listeners

[Andrew Leonard]

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_leonard at uk.ibm.com 




From:   Alan Bateman <Alan.Bateman at oracle.com>
To:     "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>, Andrew 
Leonard <andrew_m_leonard at uk.ibm.com>, serviceability-dev at 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.spitsyn at 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190610/c1f1046a/attachment.html>