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