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

[Andrew Leonard]

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




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

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