So good point, what does thread-safety mean here:
I've gone back around my change and am now satisfied just changing to use
ConcurrentHashMap is sufficient for what is required here. The
GenericListenerConnector and it's sub-class are essentially a wrapper
class to the underlying TransportService object. It essentially marshalls
the listening requests on various addresses/ports through to the
transportService. The addr:port uniqueness/clashes is ultimately delegated
to the transportService who really knows what ports are allocated, and as
long as the GenericListenerConnector only put's listener's into its map
upon a thread's successful transportService.startListening() and removes
them upon a successful transportService.stopListening(), that should work,
hence why I have left stopListening() method as it was (in webrev.02). The
real requirement of GenericListenerConnector from a thread-safety aspect
is that it safely stores/manages it's map of listeners, which is where the
bug was, in that it needed a ConcurrentHashMap. Thus get/puts.. don't
corrupt the map.
I am thus happy with my webrev.02, which is as .00 but i've updated the
testcase to allocate ports using port=0.
http://cr.openjdk.java.net/~aleonard/8225474/webrev.02/
I have now run my testcase x200 with no failure.
I have also run the suggested jshell tests numerous times all passing:
jtreg -concurrency:12 -v1 langtools/test/jdk/jshell
So I am quite happy with it as it stands, i've thrashed the startListening
concurrency quite hard on xLinux and it now seems robust.
I am going to run some stress tests on some other platforms tomorrow,
Win64 and Mac if I can.
Is one of the JDI SME's able to review this please?
Thanks
Andrew
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com
From: David Holmes <david.hol...@oracle.com>
To: Alan Bateman <alan.bate...@oracle.com>,
"serguei.spit...@oracle.com" <serguei.spit...@oracle.com>, Andrew Leonard
<andrew_m_leon...@uk.ibm.com>, serviceability-dev@openjdk.java.net
Date: 09/06/2019 14:30
Subject: Re: RFR JDK-8225474: JDI connector accept fails "Address
already in use" with concurrent listeners
+1 from me.
What thread-safety should mean here needs to be defined.
David
On 8/06/2019 7:11 pm, Alan Bateman wrote:
> 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
