In doing the recent changes I applied knowledge of how the ConnectorImpl and its defaultArguments are used to decide on what is necessary from a threading perspective. I also am only considering the "listening" concurrency issue, at this stage, and was not considering making all the jdi classes thread-safe. The defaultArguments is constructed at GenericListenerConnector constructor time, and not changed from there. The whole ConnectorImpl implementation is basically to wrapper a TransportService with a set of "default arguments" set at object construction. This design affects what is necessary from a threading perspective for the Connector instance:
- ConnectorImpl::toString iterates over the values in defaultArguments, I assume this will require synchronization => Not necessary as defaultArguments does not change after construction - ConnectorImpl.trueString and falseString are initialized lazily without synchronization. It might be okay but a comment to explain why. Ditto for the lazily initialization of the messages field in getString. => "messages" is an instance variable initialized at constructor time only => true/falseString are static, they probably should be a kin to the "messages", but since they are just "immutable" Strings I don't think it's an issue as-is - All but the "value" field in the Connector.Argument implementations should be final. It might be that the value fields need to volatile (needs more study). => Again I didn't see the essential need for this given the defaultArguments are constructed and not modified, including the "value" field. From the perspective of callers obtaining a copy-of the defaultArguments to set their own values, I saw that then as being a "thread-safety" concern of that calling code. - GenericListeningConnector fields should be final. You can also initialize listenMap with new ConcurrentHashMap<>() . => Isn't this just optimization not relevant to the "bug" in question? - GenericListeningConnector.accept needs further analysis, at least for the case where the debugger hasn't called startListening. => I did think through the accept() method logic, and the compatibility start/stopListening if needed looks fine to me I must agree that retro-fitting thread safety when not design from the start is difficult. So I therefore question the testcase, the spec doesn't state it is thread-safe, so why have we written testcases assuming it? Given the JDWP/JDI spec does it lends itself for a user to naturally assume having multiple threads listening on the SocketListen Connector is a reasonable usecase? How would a debugger font-end UI handle it? I'm not experienced enough with JDI, but my natural thought is it could be reasonable to have a "front-end" listening on multiple TargetVM's and handling them in a multi-threaded manner....? Should we re-target this as a CSR request instead? and re-implement. Cheers 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: Andrew Leonard <andrew_m_leon...@uk.ibm.com> Cc: serviceability-dev@openjdk.java.net Date: 14/06/2019 09:27 Subject: Re: RFR JDK-8225474: JDI connector accept fails "Address already in use" with concurrent listeners On 13/06/2019 20:18, Andrew Leonard wrote: Thanks Alex, good spot. I have done all the changes now with the latest updates, and have performed some stress tests on xLinux, Win64 and Mac64, all successful. Please can I get reviews and sponsor for webrev.04: http://cr.openjdk.java.net/~aleonard/8225474/webrev.04/ Retrofitting code that wasn't developed to be used by multiple concurrent threads to be thread safe takes time and will require a lot of review effort. I don't have time to spend on this now and I suspect this one will need a second reviewer to help with the confidence that you've got everything. Things to check are: - ConnectorImpl::toString iterates over the values in defaultArguments, I assume this will require synchronization - ConnectorImpl.trueString and falseString are initialized lazily without synchronization. It might be okay but a comment to explain why. Ditto for the lazily initialization of the messages field in getString. - All but the "value" field in the Connector.Argument implementations should be final. It might be that the value fields need to volatile (needs more study). - GenericListeningConnector fields should be final. You can also initialize listenMap with new ConcurrentHashMap<>() . - GenericListeningConnector.accept needs further analysis, at least for the case where the debugger hasn't called startListening. There may be more issues, it just needs time to walk through the original implementation. -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
