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

[serguei.spit...@oracle.com]

Hi Andrew,

That is Okay.
Thank you for sharing it!

Thanks,
Serguei


On 9/26/19 05:35, Andrew Leonard wrote:
I think given the complexities involved in trying to make the JDI 
connector's "thread-safe", i'm thinking now it's probably best leaving 
this as-is. As Alan pointed out the java doc does not indicate it is 
meant to be thread safe, and we will review the testcases that we have.

Thanks for all the discussion on this.
Cheers
Andrew


Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com

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

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

[Andrew Leonard]

I think given the complexities involved in trying to make the JDI 
connector's "thread-safe", i'm thinking now it's probably best leaving 
this as-is. As Alan pointed out the java doc does not indicate it is meant 
to be thread safe, and we will review the testcases that we have.
Thanks for all the discussion on this.
Cheers
Andrew


Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 

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

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

[Andrew Leonard]

Hi Alan,
Was wondering if you had had a chance to look at this please?
Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 




From:   Andrew Leonard/UK/IBM
To: Alan Bateman 
Cc: serviceability-dev@openjdk.java.net
Date:   02/07/2019 08:37
Subject:Re: RFR JDK-8225474: JDI connector accept fails "Address 
already in use" with concurrent listeners


Thanks Alan, much appreciated.

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 





From:   Alan Bateman 
To: Andrew Leonard 
Cc: serviceability-dev@openjdk.java.net
Date:   02/07/2019 07:45
Subject:        Re: RFR JDK-8225474: JDI connector accept fails "Address 
already in use" with concurrent listeners



On 01/07/2019 20:41, Andrew Leonard wrote:
Any one able to review please? 
This one will take a significant time to review. We also need to figure 
out if an @implNote if needed as nobody using these connectors will know 
(from the javadoc) that some of the implementations are thread safe. I 
hope to get to it in the next few weeks.

-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


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

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

[Andrew Leonard]

Thanks Alan, much appreciated.

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 




From:   Alan Bateman 
To: Andrew Leonard 
Cc: serviceability-dev@openjdk.java.net
Date:   02/07/2019 07:45
Subject:Re: RFR JDK-8225474: JDI connector accept fails "Address 
already in use" with concurrent listeners



On 01/07/2019 20:41, Andrew Leonard wrote:
Any one able to review please? 
This one will take a significant time to review. We also need to figure 
out if an @implNote if needed as nobody using these connectors will know 
(from the javadoc) that some of the implementations are thread safe. I 
hope to get to it in the next few weeks.

-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

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

[Alan Bateman]

On 01/07/2019 20:41, Andrew Leonard wrote:

Any one able to review please?
This one will take a significant time to review. We also need to figure 
out if an @implNote if needed as nobody using these connectors will know 
(from the javadoc) that some of the implementations are thread safe. I 
hope to get to it in the next few weeks.


-Alan.

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

[Andrew Leonard]

Any one able to review please?
Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 




From:   Andrew Leonard/UK/IBM
To: Alan Bateman 
Cc: serviceability-dev@openjdk.java.net
Date:   19/06/2019 18:29
Subject:Re: RFR JDK-8225474: JDI connector accept fails "Address 
already in use" with concurrent listeners


Alan and other reviewers please?,
Firstly apologies for the long post, but I wanted to be thorough.

Following previous comments I have reviewed my understanding of 
concurrency and "safe publishing" in the JMM, and I have a fairly clear 
understanding now. I have reworked my fix to the issue associated with 
concurrent JDI connector listeners after thoroughly reviewing and walking 
through the sun.tools.jdi Connector implementation and also reading the 
spec javadoc.
My Goal: To make JDI Connector and associated listening/attaching APIs 
"thread-safe", for the purpose of supporting a debugger use case where 
multiple listening/attaching "threads" are used.

>From the review I determined the following key components are already 
thread-safe:
- VirtualMachineImpl
- VirtualMachineManagerImpl

The components that needed some work were:
- ConnectorImpl and all its sub-classes
- A Minor fix to SocketTransportService

My new patch is available for review here: 
http://cr.openjdk.java.net/~aleonard/8225474/webrev.05/

Tests run successfully:
- new JdwpConcurrentAttachTest.java which performs a multi-threaded stress 
test of the start/stopListening process where the "bug" occurs
- jtreg -jdk:$PRODUCT_HOME -concurrency:12 -v1 -timeoutfactor:20 
langtools/jdk/jshell
- jtreg -jdk:$PRODUCT_HOME -timeoutfactor:20 -v1 jdk/com/sun/jdi  (2 tests 
failed that failed before for unrelated reasons)

The following summarizes the key changes and why:

ConnectorImpl:
final Map defaultArguments = new LinkedHashMap<>();
==> Made "final" so that Connector object defaultArguments is 
safely published to other threads after construction

defaultArguments():
+   synchronized(defaultArguments) {
Collection values = defaultArguments.values();
==> Synchronize on defaultArguments before iterating over values to 
return a "copy" to debugger

addString/Boolean/Integer/SelectedArgument:
+synchronized(defaultArguments) {
 defaultArguments.put(name,
==> synchronize on defaultArguments for updates

toString:
+synchronized(defaultArguments) {
Iterator iter = 
defaultArguments().values().iterator();
==> synchronize on defaultArguments prior to values iteration, and to 
create a "happens-before" relationship

+   synchronized(this) {
if (messages == null) {
messages = 
ResourceBundle.getBundle("com.sun.tools.jdi.resources.jdi");
}
+   }
==> Protect messages construction

ArgumentImpl
private final String name;
private final String label;
private final String description;
private volatile String value;
private final boolean mustSpecify;
==> final all except value which is volatile, so that Argument can 
be safely published to other threads after construction
==> value is "volatile" as this can be set by debuggers, so must 
set volatile to ensure if passed to other thread a "happens-before" is 
ensured

BooleanArgumentImpl
+synchronized(ConnectorImpl.class) {
if(trueString == null) {
trueString = getString("true");
falseString = getString("false");
}
+}
==> synchronized on ConnectorImpl.class object to ensure lock for 
initializing the static fields


GenericListeningConnector: 
final Map, 
TransportService.ListenKey>  listenMap;
final TransportService transportService;
final Transport transport;
==> "final" these to ensure "safe publication" to other threads 
after Connector construction

 
private GenericListeningConnector(TransportService ts,
   boolean addAddressArgument,
+  Transport tsp)
 ==> Added Transport param to constructor so that sub-classes can 
pass in their desired Transport and then allow transport to be "final" for 
"safe publication". Previously transport was being constructed "twice" 
wastefully.
 
listenMap = new ConcurrentHashMap, TransportService.ListenKey>(10);
==> Make listenMap a ConcurrentHashMap so that it is "thread-safe" 
for access/updates

 
public synchronized String startListening/stopListening(..
==> Made start & stop listening "synchronized&quo

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

[Andrew Leonard]

Alan and other reviewers please?,
Firstly apologies for the long post, but I wanted to be thorough.

Following previous comments I have reviewed my understanding of 
concurrency and "safe publishing" in the JMM, and I have a fairly clear 
understanding now. I have reworked my fix to the issue associated with 
concurrent JDI connector listeners after thoroughly reviewing and walking 
through the sun.tools.jdi Connector implementation and also reading the 
spec javadoc.
My Goal: To make JDI Connector and associated listening/attaching APIs 
"thread-safe", for the purpose of supporting a debugger use case where 
multiple listening/attaching "threads" are used.

>From the review I determined the following key components are already 
thread-safe:
- VirtualMachineImpl
- VirtualMachineManagerImpl

The components that needed some work were:
- ConnectorImpl and all its sub-classes
- A Minor fix to SocketTransportService

My new patch is available for review here: 
http://cr.openjdk.java.net/~aleonard/8225474/webrev.05/

Tests run successfully:
- new JdwpConcurrentAttachTest.java which performs a multi-threaded stress 
test of the start/stopListening process where the "bug" occurs
- jtreg -jdk:$PRODUCT_HOME -concurrency:12 -v1 -timeoutfactor:20 
langtools/jdk/jshell
- jtreg -jdk:$PRODUCT_HOME -timeoutfactor:20 -v1 jdk/com/sun/jdi  (2 tests 
failed that failed before for unrelated reasons)

The following summarizes the key changes and why:

ConnectorImpl:
final Map defaultArguments = new LinkedHashMap<>();
==> Made "final" so that Connector object defaultArguments is 
safely published to other threads after construction

defaultArguments():
+   synchronized(defaultArguments) {
Collection values = defaultArguments.values();
==> Synchronize on defaultArguments before iterating over values to 
return a "copy" to debugger

addString/Boolean/Integer/SelectedArgument:
+synchronized(defaultArguments) {
 defaultArguments.put(name,
==> synchronize on defaultArguments for updates

toString:
+synchronized(defaultArguments) {
Iterator iter = 
defaultArguments().values().iterator();
==> synchronize on defaultArguments prior to values iteration, and to 
create a "happens-before" relationship

+   synchronized(this) {
if (messages == null) {
messages = 
ResourceBundle.getBundle("com.sun.tools.jdi.resources.jdi");
}
+   }
==> Protect messages construction

ArgumentImpl
private final String name;
private final String label;
private final String description;
private volatile String value;
private final boolean mustSpecify;
==> final all except value which is volatile, so that Argument can 
be safely published to other threads after construction
==> value is "volatile" as this can be set by debuggers, so must 
set volatile to ensure if passed to other thread a "happens-before" is 
ensured

BooleanArgumentImpl
+synchronized(ConnectorImpl.class) {
if(trueString == null) {
trueString = getString("true");
falseString = getString("false");
}
+}
==> synchronized on ConnectorImpl.class object to ensure lock for 
initializing the static fields


GenericListeningConnector: 
final Map, 
TransportService.ListenKey>  listenMap;
final TransportService transportService;
final Transport transport;
==> "final" these to ensure "safe publication" to other threads 
after Connector construction

 
private GenericListeningConnector(TransportService ts,
   boolean addAddressArgument,
+  Transport tsp)
 ==> Added Transport param to constructor so that sub-classes can 
pass in their desired Transport and then allow transport to be "final" for 
"safe publication". Previously transport was being constructed "twice" 
wastefully.
 
listenMap = new ConcurrentHashMap, TransportService.ListenKey>(10);
==> Make listenMap a ConcurrentHashMap so that it is "thread-safe" 
for access/updates

 
public synchronized String startListening/stopListening(..
==> Made start & stop listening "synchronized" so that management 
of listenMap entry and transportService state are locked&synchronized

 
GenericAttachingConnector:
final TransportService transportService;
final Transport transport;
==> Made "final" so that Connector can be safely published

 private GenericAttachingConnector(TransportService ts,
   boolean addAddressArgument,
+  Transport tsp)
==> Added Transport param to constructor so that sub-classes can 
pass in their desired Transport and then allow transport to be "final" for 
"safe publication". Previously transport was being constructed "twice" 
wa

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

[Alan Bateman]

On 17/06/2019 11:14, Andrew Leonard wrote:
Thanks for the feedback Alan. I think i'm going to have to defer on 
this issue for the moment, as I dont' have enough experience with JDI 
usecases to accurately study this at the moment. I think you have also 
demonstrated my "rustiness" in thread-safety issues, I think I will go 
and brush up!!


Should we create a separate bug to update the spec/doc to clarify the 
position on JDI Connectors being non-thread-safe, or is the current 
non-statement the normal stance on this one?
It wouldn't do any harm to add something to the javadoc to make it clear 
that Connectors are not safe for use by multiple concurrent threads and 
so need appropriate synchronization. There are "Thread safety" sections 
in several classes and something similar wouldn't look out of place here.


-Alan.

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

[Andrew Leonard]

I'm going to persevere with this a bit more..
but first I need to better understand "safe publication"

Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 




From:   Alan Bateman 
To: Andrew Leonard 
Cc: serviceability-dev@openjdk.java.net
Date:   14/06/2019 17:06
Subject:        Re: RFR JDK-8225474: JDI connector accept fails "Address 
already in use" with concurrent listeners



On 14/06/2019 15:52, Andrew Leonard wrote:
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. 
Sure but default arguments exposed in the ListeningConnector API are 
mutable. It may be that debuggers and other tools aren't changing them but 
we can't say that the GenericListeningConnector and its implementations 
are safe for by use by concurrent threads without studying this further. 
Also the comments about changing fields to final are not optimizations, 
that are suggestions to allow GenericListeningConnector objects be safely 
published.

-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

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

[Andrew Leonard]

Thanks for the feedback Alan. I think i'm going to have to defer on this 
issue for the moment, as I dont' have enough experience with JDI usecases 
to accurately study this at the moment. I think you have also demonstrated 
my "rustiness" in thread-safety issues, I think I will go and brush up!!

Should we create a separate bug to update the spec/doc to clarify the 
position on JDI Connectors being non-thread-safe, or is the current 
non-statement the normal stance on this one?

Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 




From:   Alan Bateman 
To: Andrew Leonard 
Cc: serviceability-dev@openjdk.java.net
Date:   14/06/2019 17:06
Subject:    Re: RFR JDK-8225474: JDI connector accept fails "Address 
already in use" with concurrent listeners



On 14/06/2019 15:52, Andrew Leonard wrote:
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. 
Sure but default arguments exposed in the ListeningConnector API are 
mutable. It may be that debuggers and other tools aren't changing them but 
we can't say that the GenericListeningConnector and its implementations 
are safe for by use by concurrent threads without studying this further. 
Also the comments about changing fields to final are not optimizations, 
that are suggestions to allow GenericListeningConnector objects be safely 
published.

-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

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

[Alan Bateman]

On 14/06/2019 15:52, Andrew Leonard wrote:
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. 
Sure but default arguments exposed in the ListeningConnector API are 
mutable. It may be that debuggers and other tools aren't changing them 
but we can't say that the GenericListeningConnector and its 
implementations are safe for by use by concurrent threads without 
studying this further. Also the comments about changing fields to final 
are not optimizations, that are suggestions to allow 
GenericListeningConnector objects be safely published.


-Alan

Re: 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_leon...@uk.ibm.com 




From:   Alan Bateman 
To: Andrew Leonard 
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

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

[Alan Bateman]

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

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

[Andrew Leonard]

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/

Many Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 




From:   Alex Menkov 
To: "serguei.spit...@oracle.com" , Andrew 
Leonard 
Cc: serviceability-dev@openjdk.java.net
Date:   13/06/2019 18:15
Subject:        Re: RFR JDK-8225474: JDI connector accept fails "Address 
already in use" with concurrent listeners



One more minor note for the test:

@build HelloWorld JdwpConcurrentAttachTest

The test does not use HelloWorld class

--alex

On 06/12/2019 12:52, serguei.spit...@oracle.com wrote:
> Hi Andrew,
> 
> It looks good.
> 
> Minor comments on the test.
> 
> There are several places where white spaces are missed around the signs: 

> '=', '<', '+', ',':
> 
>58 for(int j=0; j<10; j++) {
> 
>62 int numThreads=10;
> 
>66 for(int i=0; i 
>67 JdwpConcurrentAttachTest test = new 
JdwpConcurrentAttachTest(localAddresses[0].getHostAddress(),0);
> 
>78
>79 static  boolean   failure=false;
>80 static  Exception failureException=null;
> 
>   112 log("Listening address = "+actualAddress);
> 
>   123 failureException=e;
> 
>   132 +",address="+ actualAddress
> 
> 
> 
> 
> 
> On 6/11/19 12:36, Andrew Leonard wrote:
>> Thanks Alex,
>> I have changed the testcase to allocate ports which makes better 
>> sense: 
https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Ealeonard_8225474_webrev.02_&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=NaV8Iy8Ld-vjpXZFDdTbgGlRTghGHnwM75wUPd5_NUQ&m=p--c3Hawio2kCsD0eo4leL8TLWZPtlYui8rmDo0E7So&s=EnJJuj3Yg8BeU7c2Fn6cqEeJGa6C5e4pIWGESAjwfhc&e=
 

>> Cheers
>> Andrew
>>
>> Andrew Leonard
>> Java Runtimes Development
>> IBM Hursley
>> IBM United Kingdom Ltd
>> internet email: andrew_m_leon...@uk.ibm.com
>>
>>
>>
>>
>> From: Alex Menkov 
>> To: Andrew Leonard 
>> Cc: serviceability-dev@openjdk.java.net
>> Date: 10/06/2019 20:43
>> Subject: Re: RFR JDK-8225474: JDI connector accept fails "Address 
>> already in use" with concurrent listeners
>> 

>>
>>
>>
>>
>>
>> On 06/10/2019 02:07, Andrew Leonard wrote:
>> > Hi Alex,
>> > Thanks for trying this out, it's possible this failure is an issue 
with
>> > my testcase, as I repeat the 10 thread test loop 10 times and I have 
a
>> > 30 second timeout which potentially might overrun if a connection 
takes
>> > time to terminate. I could avoid that simply by using different ports
>> > for each run, equally it could also be an issue as pointed out by 
Alan.
>>
>> The test uses fixed ports, so it can interfere with other processes.
>> You can use port "0" which automatically selects free port.
>> connector.startListening returns actual port assigned for the 
connection.
>>
>> > Thanks
>> > Andrew
>> >
>> > Andrew Leonard
>> > Java Runtimes Development
>> > IBM Hursley
>> > IBM United Kingdom Ltd
>> > internet email: andrew_m_leon...@uk.ibm.com
>> >
>> >
>> >
>> >
>> > From: Alex Menkov 
>> > To: Andrew Leonard ,
>> > serviceability-dev@openjdk.java.net
>> > Date: 07/06/2019 22:19
>> > Subject: Re: RFR JDK-8225474: JDI connector accept fails "Address
>> > already in use" with concurrent listeners
>> > 

>> >
>> >
>> >
>> > Hi Andrew,
>> >
>> > The fix looks good in general.
>> > And I can be the sponsor.
>> > I've tested the fix and got 1 test failure (of 400 runs) on Win-x64 
and
>> > the log looks the same.
>> >
>> > java.lang.RuntimeException: Failed to accept connector connection
>> > at JdwpConcurrentAttachTest.main(JdwpConcurrentAttachTest.java:74)
>> > at
>> > 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
>> > Method)
>> > at
>> > 
>> 

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

[Alex Menkov]

One more minor note for the test:

@build HelloWorld JdwpConcurrentAttachTest

The test does not use HelloWorld class

--alex

On 06/12/2019 12:52, serguei.spit...@oracle.com wrote:

Hi Andrew,

It looks good.

Minor comments on the test.

There are several places where white spaces are missed around the signs: 
'=', '<', '+', ',':


   58 for(int j=0; j<10; j++) {

   62 int numThreads=10;

   66 for(int i=0; i
Thanks Alex,
I have changed the testcase to allocate ports which makes better 
sense: http://cr.openjdk.java.net/~aleonard/8225474/webrev.02/

Cheers
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com




From: Alex Menkov 
To: Andrew Leonard 
Cc: serviceability-dev@openjdk.java.net
Date: 10/06/2019 20:43
Subject: Re: RFR JDK-8225474: JDI connector accept fails "Address 
already in use" with concurrent listeners







On 06/10/2019 02:07, Andrew Leonard wrote:
> Hi Alex,
> Thanks for trying this out, it's possible this failure is an issue with
> my testcase, as I repeat the 10 thread test loop 10 times and I have a
> 30 second timeout which potentially might overrun if a connection takes
> time to terminate. I could avoid that simply by using different ports
> for each run, equally it could also be an issue as pointed out by Alan.

The test uses fixed ports, so it can interfere with other processes.
You can use port "0" which automatically selects free port.
connector.startListening returns actual port assigned for the connection.

> Thanks
> Andrew
>
> Andrew Leonard
> Java Runtimes Development
> IBM Hursley
> IBM United Kingdom Ltd
> internet email: andrew_m_leon...@uk.ibm.com
>
>
>
>
> From: Alex Menkov 
> To: Andrew Leonard ,
> serviceability-dev@openjdk.java.net
> Date: 07/06/2019 22:19
> Subject: Re: RFR JDK-8225474: JDI connector accept fails "Address
> already in use" with concurrent listeners
> 
>
>
>
> Hi Andrew,
>
> The fix looks good in general.
> And I can be the sponsor.
> I've tested the fix and got 1 test failure (of 400 runs) on Win-x64 and
> the log looks the same.
>
> java.lang.RuntimeException: Failed to accept connector connection
> at JdwpConcurrentAttachTest.main(JdwpConcurrentAttachTest.java:74)
> at
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
> Method)
> at
> 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)

> at
> 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

> at java.base/java.lang.reflect.Method.invoke(Method.java:567)
> at
> 
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)

> at java.base/java.lang.Thread.run(Thread.java:830)
> Caused by: java.net.BindException: Address already in use: bind
> at java.base/sun.nio.ch.Net.bind0(Native Method)
> at java.base/sun.nio.ch.Net.bind(Net.java:465)
> at java.base/sun.nio.ch.Net.bind(Net.java:457)
> at java.base/sun.nio.ch.NioSocketImpl.bind(NioSocketImpl.java:643)
> at java.base/java.net.ServerSocket.bind(ServerSocket.java:359)
> at java.base/java.net.ServerSocket.bind(ServerSocket.java:313)
> at
> 
jdk.jdi/com.sun.tools.jdi.SocketTransportService.startListening(SocketTransportService.java:300)

> at
> 
jdk.jdi/com.sun.tools.jdi.SocketTransportService.startListening(SocketTransportService.java:310)

> at
> 
jdk.jdi/com.sun.tools.jdi.GenericListeningConnector.startListening(GenericListeningConnector.java:117)

> at
> 
jdk.jdi/com.sun.tools.jdi.SocketListeningConnector.startListening(SocketListeningConnector.java:86)
> at 
JdwpConcurrentAttachTest.attachTest(JdwpConcurrentAttachTest.java:111)

> at JdwpConcurrentAttachTest.run(JdwpConcurrentAttachTest.java:93)
>
> So looks like the fix doesn't fix the issue completely.
>
> --alex
>
> On 06/07/2019 07:24, Andrew Leonard wrote:
>> Hi,
>> Please can I request a sponsor and review of this patch for 
JDK-8225474.

>> A problem that we have seen intermittently with JDI connector stress
>> tests for quite a while that is caused by an non-thread safe 
HashMap  in

>> the connector class. I've created a standalone testcase that reproduces
>> it reliably.
>> http://cr.openjdk.java.net/~aleonard/8225474/webrev.00
>>
>> Thanks
>> Andrew
>>
>>
>> Andrew Leonard
>> Java Runtimes Development
>> IBM Hursley
>> IBM United Kingdom Ltd
>> internet email: andrew_m_leon...@uk.i

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

[Andrew Leonard]

Thanks Serguei, i'll add those changes to my new webrev I am working on 
based on Alan's suggestions. It should be ready once i've finished my 
stress tests later today.
Cheers
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 




From:   "serguei.spit...@oracle.com" 
To: Andrew Leonard , Alex Menkov 

Cc: serviceability-dev@openjdk.java.net
Date:   12/06/2019 20:52
Subject:        Re: RFR JDK-8225474: JDI connector accept fails "Address 
already in use" with concurrent listeners



Hi Andrew,

It looks good.

Minor comments on the test.

There are several places where white spaces are missed around the signs: 
'=', '<', '+', ',':

  58 for(int j=0; j<10; j++) {
  62 int numThreads=10;
  66 for(int i=0; ihttp://cr.openjdk.java.net/~aleonard/8225474/webrev.02/ 
Cheers 
Andrew 

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 




From:Alex Menkov  
To:Andrew Leonard  
Cc:serviceability-dev@openjdk.java.net 
Date:    10/06/2019 20:43 
Subject:    Re: RFR JDK-8225474: JDI connector accept fails "Address 
already in use" with concurrent listeners 





On 06/10/2019 02:07, Andrew Leonard wrote:
> Hi Alex,
> Thanks for trying this out, it's possible this failure is an issue with 
> my testcase, as I repeat the 10 thread test loop 10 times and I have a 
> 30 second timeout which potentially might overrun if a connection takes 
> time to terminate. I could avoid that simply by using different ports 
> for each run, equally it could also be an issue as pointed out by Alan.

The test uses fixed ports, so it can interfere with other processes.
You can use port "0" which automatically selects free port.
connector.startListening returns actual port assigned for the connection.

> Thanks
> Andrew
> 
> Andrew Leonard
> Java Runtimes Development
> IBM Hursley
> IBM United Kingdom Ltd
> internet email: andrew_m_leon...@uk.ibm.com
> 
> 
> 
> 
> From: Alex Menkov 
> To: Andrew Leonard , 
> serviceability-dev@openjdk.java.net
> Date: 07/06/2019 22:19
> Subject: Re: RFR JDK-8225474: JDI connector accept fails "Address 
> already in use" with concurrent listeners
> 
> 
> 
> 
> Hi Andrew,
> 
> The fix looks good in general.
> And I can be the sponsor.
> I've tested the fix and got 1 test failure (of 400 runs) on Win-x64 and
> the log looks the same.
> 
> java.lang.RuntimeException: Failed to accept connector connection
> at JdwpConcurrentAttachTest.main(JdwpConcurrentAttachTest.java:74)
> at
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
> Method)
> at
> 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> at
> 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.base/java.lang.reflect.Method.invoke(Method.java:567)
> at
> 
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
> at java.base/java.lang.Thread.run(Thread.java:830)
> Caused by: java.net.BindException: Address already in use: bind
> at java.base/sun.nio.ch.Net.bind0(Native Method)
> at java.base/sun.nio.ch.Net.bind(Net.java:465)
> at java.base/sun.nio.ch.Net.bind(Net.java:457)
> at java.base/sun.nio.ch.NioSocketImpl.bind(NioSocketImpl.java:643)
> at java.base/java.net.ServerSocket.bind(ServerSocket.java:359)
> at java.base/java.net.ServerSocket.bind(ServerSocket.java:313)
> at
> 
jdk.jdi/com.sun.tools.jdi.SocketTransportService.startListening(SocketTransportService.java:300)
> at
> 
jdk.jdi/com.sun.tools.jdi.SocketTransportService.startListening(SocketTransportService.java:310)
> at
> 
jdk.jdi/com.sun.tools.jdi.GenericListeningConnector.startListening(GenericListeningConnector.java:117)
> at
> 
jdk.jdi/com.sun.tools.jdi.SocketListeningConnector.startListening(SocketListeningConnector.java:86)
> at 
JdwpConcurrentAttachTest.attachTest(JdwpConcurrentAttachTest.java:111)
> at JdwpConcurrentAttachTest.run(JdwpConcurrentAttachTest.java:93)
> 
> So looks like the fix doesn't fix the issue completely.
> 
> --alex
> 
> On 06/07/2019 07:24, Andrew Leonard wrote:
>> Hi,
>> Please can I request a sponsor and review of this patch for 
JDK-8225474.
>> A problem that we have seen intermittently with JDI connector stress
>> tests for quite a while that is caused by an non-thread safe HashMap 
 in
>> the connector class. I've created a standalone testcase that repr

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

[serguei.spit...@oracle.com]

  
  
Hi Andrew,
  
  It looks good.
  
  Minor comments on the test.
  
  There are several places where white spaces are missed around the
  signs: '=', '<', '+', ',':
  
58 for(int j=0; j<10; j++) {
62 int numThreads=10;
66 for(int i=0; i
67 JdwpConcurrentAttachTest test = new JdwpConcurrentAttachTest(localAddresses[0].getHostAddress(),0);
78 
  79 static  boolean   failure=false;
  80 static  Exception failureException=null;

 112 log("Listening address = "+actualAddress);

 123 failureException=e;

 132 +",address="+ actualAddress
  
  
  
  
  On 6/11/19 12:36, Andrew Leonard wrote:

Thanks
Alex,
  
  I have changed
the testcase to allocate ports which makes better sense: http://cr.openjdk.java.net/~aleonard/8225474/webrev.02/
  
  Cheers
  
  Andrew
  
  
  Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 
  
  
  
  
  
  From:
       Alex
Menkov 
  
  To:
       Andrew
Leonard 
  
  Cc:
       serviceability-dev@openjdk.java.net
  
  Date:
       10/06/2019
        20:43
      
  Subject:
           Re:
        RFR JDK-8225474: JDI connector accept fails "Address already in
use"
with concurrent listeners
  
  
  
  
  
  
  
  On 06/10/2019 02:07, Andrew Leonard wrote:
  > Hi Alex,
  > Thanks for trying this out, it's possible this failure is
  an issue
  with 
  > my testcase, as I repeat the 10 thread test loop 10 times
  and I have
  a 
  > 30 second timeout which potentially might overrun if a
  connection
  takes 
  > time to terminate. I could avoid that simply by using
  different ports
  
  > for each run, equally it could also be an issue as
  pointed out by
  Alan.
  
  The test uses fixed ports, so it can interfere with other
  processes.
  You can use port "0" which automatically selects free port.
  connector.startListening returns actual port assigned for the
  connection.
  
  > Thanks
  > Andrew
  > 
  > Andrew Leonard
  > Java Runtimes Development
  > IBM Hursley
  > IBM United Kingdom Ltd
  > internet email: andrew_m_leon...@uk.ibm.com
  > 
  > 
  > 
  > 
  > From: Alex Menkov 
      > To: Andrew Leonard , 
  > serviceability-dev@openjdk.java.net
          > Date: 07/06/2019 22:19
  > Subject: Re: RFR JDK-8225474: JDI connector accept fails
  "Address
  
  > already in use" with concurrent listeners
  >
  
  > 
  > 
  > 
  > Hi Andrew,
  > 
  > The fix looks good in general.
  > And I can be the sponsor.
  > I've tested the fix and got 1 test failure (of 400 runs)
  on Win-x64
  and
  > the log looks the same.
  > 
  > java.lang.RuntimeException: Failed to accept connector
  connection
  > at
  JdwpConcurrentAttachTest.main(JdwpConcurrentAttachTest.java:74)
  > at
  >
  java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
  > Method)
  > at
  >
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
  > at
  >
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
  > at
  java.base/java.lang.reflect.Method.invoke(Method.java:567)
  > at
  >
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
  > at java.base/java.lang.Thread.run(Thread.java:830)
  > Caused by: java.net.BindException: Address already in
  use: bind
  > at java.base/sun.nio.ch.Net.bind0(Native Method)
  > at java.base/sun.nio.ch.Net.bind(Net.java:465)
  > at java.base/sun.nio.ch.Net.bind(Net.java:457)
  > at
  java.base/sun.nio.ch.NioSocketImpl.bind(NioSocketImpl.java:643)
  > at
 

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

[Andrew Leonard]

Hi Alan,
I've thought about this some more, and you're right there are some edge 
cases where I think we really need to synchronize the start/stopListening. 
My initial thought the transportService would manage it, is flawed. For 
example in the potential situation where  dynamic port=0 allocation is 
used, and the transportService allocates ports, and a current thread using 
port A calls stopListening(), lets say that gets as far through that 
method as completing the transportService.stopListening() but not as far 
as the listenMap.remove(args), and then gets paused. Another thread calls 
startListening() and the transportService allocates the same port A to it 
and it completes the listenMap.put(args). The other thread then resumes 
and calls listenMap.remove(args), then removing the in-use port A of the 
other thread from the listenMap, thus causing an exception in accept() or 
stopListening()... and leaving a port in-use...

ConnectorImpl manages the defaultArguments map, which is an unaltered 
insertion-ordered linked map after object contruction. After construction 
it is purely for default arguments access, so in theory does not need 
synchronization, but all it would take is a scenario to be added for 
example to change a "default arg". So I think for safety making it 
Collections.synchronizedMap(new LinkedHashMap()); I think would make 
sense.

I shall work on a webrev.03 with these updates and perform some tests on 
xLinux, Win64 and mac64.

Thanks again
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 




From:   Alan Bateman 
To: Andrew Leonard 
Cc: serviceability-dev@openjdk.java.net
Date:   12/06/2019 10:54
Subject:    Re: RFR JDK-8225474: JDI connector accept fails "Address 
already in use" with concurrent listeners



On 10/06/2019 09:30, Andrew Leonard wrote:
Thanks for the feedback everyone. 

Hi Alan, yes I can put some cycles into digging deeper with this one, as 
you point out there's probably more that needs doing. 
As Alex found running the new testcase 400 times it failed once still, 
interestingly during initial startListening() rather than the accept() 
where it was failing before. 
On a general issue of "thread-safety", I had a good look through the spec 
docs and architecture docs and couldn't find any specification of 
thread-safety(or not), should it be inferred if the docs don't 
specifically state "thread-safe" that it should be assumed as "not-thread 
safe"? 
Right, Connectors aren't specified to be thread safe so any test that 
assumes otherwise is an issue (there may be a second issue here with a 
test using a fixed TCP port, it sounds like you might be running into both 
issues).

I've no objection to changing GenericListeningConnector to be thread safe. 
Also no issue with exploring the impact of a spec change too. Although 
Connectors (and transports) are pluggable, there probably aren't too many 
implementations outside of the JDK so you might have some scope to go 
beyond recommending that implementations be thread safe.

I've read your other mails where you've done some exploration into 
startListening/stopListening but I think there is more to do. Minimally I 
think startListening will need synchronization, alternatively changed to 
use putIfAbsent and stop the transport listening if it looses the race. 
There is also an issue with stopListening. It requires looking at the 
super class too because ConnectorImpl is not thread safe, esp. the 
defaultArguments which is a separate mutable map to listenMap.

-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

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

[Alan Bateman]

On 10/06/2019 09:30, Andrew Leonard wrote:

Thanks for the feedback everyone.

Hi Alan, yes I can put some cycles into digging deeper with this one, 
as you point out there's probably more that needs doing.
As Alex found running the new testcase 400 times it failed once still, 
interestingly during initial startListening() rather than the accept() 
where it was failing before.
On a general issue of "thread-safety", I had a good look through the 
spec docs and architecture docs and couldn't find any specification of 
thread-safety(or not), should it be inferred if the docs don't 
specifically state "thread-safe" that it should be assumed as 
"not-thread safe"?
Right, Connectors aren't specified to be thread safe so any test that 
assumes otherwise is an issue (there may be a second issue here with a 
test using a fixed TCP port, it sounds like you might be running into 
both issues).


I've no objection to changing GenericListeningConnector to be thread 
safe. Also no issue with exploring the impact of a spec change too. 
Although Connectors (and transports) are pluggable, there probably 
aren't too many implementations outside of the JDK so you might have 
some scope to go beyond recommending that implementations be thread safe.


I've read your other mails where you've done some exploration into 
startListening/stopListening but I think there is more to do. Minimally 
I think startListening will need synchronization, alternatively changed 
to use putIfAbsent and stop the transport listening if it looses the 
race. There is also an issue with stopListening. It requires looking at 
the super class too because ConnectorImpl is not thread safe, esp. the 
defaultArguments which is a separate mutable map to listenMap.


-Alan

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

[Andrew Leonard]

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 
To: Alan Bateman , 
"serguei.spit...@oracle.com" , Andrew Leonard 
, 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

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

[Andrew Leonard]

Thanks Alex,
I have changed the testcase to allocate ports which makes better sense: 
http://cr.openjdk.java.net/~aleonard/8225474/webrev.02/
Cheers
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 




From:   Alex Menkov 
To: Andrew Leonard 
Cc: serviceability-dev@openjdk.java.net
Date:   10/06/2019 20:43
Subject:Re: RFR JDK-8225474: JDI connector accept fails "Address 
already in use" with concurrent listeners





On 06/10/2019 02:07, Andrew Leonard wrote:
> Hi Alex,
> Thanks for trying this out, it's possible this failure is an issue with 
> my testcase, as I repeat the 10 thread test loop 10 times and I have a 
> 30 second timeout which potentially might overrun if a connection takes 
> time to terminate. I could avoid that simply by using different ports 
> for each run, equally it could also be an issue as pointed out by Alan.

The test uses fixed ports, so it can interfere with other processes.
You can use port "0" which automatically selects free port.
connector.startListening returns actual port assigned for the connection.

> Thanks
> Andrew
> 
> Andrew Leonard
> Java Runtimes Development
> IBM Hursley
> IBM United Kingdom Ltd
> internet email: andrew_m_leon...@uk.ibm.com
> 
> 
> 
> 
> From: Alex Menkov 
> To: Andrew Leonard , 
> serviceability-dev@openjdk.java.net
> Date: 07/06/2019 22:19
> Subject: Re: RFR JDK-8225474: JDI connector accept fails "Address 
> already in use" with concurrent listeners
> 
> 
> 
> 
> Hi Andrew,
> 
> The fix looks good in general.
> And I can be the sponsor.
> I've tested the fix and got 1 test failure (of 400 runs) on Win-x64 and
> the log looks the same.
> 
> java.lang.RuntimeException: Failed to accept connector connection
> at JdwpConcurrentAttachTest.main(JdwpConcurrentAttachTest.java:74)
> at
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
> Method)
> at
> 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> at
> 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.base/java.lang.reflect.Method.invoke(Method.java:567)
> at
> 
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
> at java.base/java.lang.Thread.run(Thread.java:830)
> Caused by: java.net.BindException: Address already in use: bind
> at java.base/sun.nio.ch.Net.bind0(Native Method)
> at java.base/sun.nio.ch.Net.bind(Net.java:465)
> at java.base/sun.nio.ch.Net.bind(Net.java:457)
> at java.base/sun.nio.ch.NioSocketImpl.bind(NioSocketImpl.java:643)
> at java.base/java.net.ServerSocket.bind(ServerSocket.java:359)
> at java.base/java.net.ServerSocket.bind(ServerSocket.java:313)
> at
> 
jdk.jdi/com.sun.tools.jdi.SocketTransportService.startListening(SocketTransportService.java:300)
> at
> 
jdk.jdi/com.sun.tools.jdi.SocketTransportService.startListening(SocketTransportService.java:310)
> at
> 
jdk.jdi/com.sun.tools.jdi.GenericListeningConnector.startListening(GenericListeningConnector.java:117)
> at
> 
jdk.jdi/com.sun.tools.jdi.SocketListeningConnector.startListening(SocketListeningConnector.java:86)
> at 
JdwpConcurrentAttachTest.attachTest(JdwpConcurrentAttachTest.java:111)
> at JdwpConcurrentAttachTest.run(JdwpConcurrentAttachTest.java:93)
> 
> So looks like the fix doesn't fix the issue completely.
> 
> --alex
> 
> On 06/07/2019 07:24, Andrew Leonard wrote:
>> Hi,
>> Please can I request a sponsor and review of this patch for 
JDK-8225474.
>> A problem that we have seen intermittently with JDI connector stress
>> tests for quite a while that is caused by an non-thread safe HashMap in
>> the connector class. I've created a standalone testcase that reproduces
>> it reliably.
>> 
https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Ealeonard_8225474_webrev.00&d=DwIC-g&c=jf_iaSHvJObTbx-siA1ZOg&r=NaV8Iy8Ld-vjpXZFDdTbgGlRTghGHnwM75wUPd5_NUQ&m=OaT9vhOgj2gp0DQBwJgmItAjmzP-GsUEkGJAVbN9ZCE&s=EQwT-4TnAIqImG1-KTOT803z1DRjmTTeMtNV-9S9-6M&e=
 

>> 
>> Thanks
>> Andrew
>> 
>> 
>> Andrew Leonard
>> Java Runtimes Development
>> IBM Hursley
>> IBM United Kingdom Ltd
>> internet email: andrew_m_leon...@uk.ibm.com
>> 
>> 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
> 
> 
> 
> 
> 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




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

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

[Alex Menkov]

On 06/10/2019 02:07, Andrew Leonard wrote:

Hi Alex,
Thanks for trying this out, it's possible this failure is an issue with 
my testcase, as I repeat the 10 thread test loop 10 times and I have a 
30 second timeout which potentially might overrun if a connection takes 
time to terminate. I could avoid that simply by using different ports 
for each run, equally it could also be an issue as pointed out by Alan.


The test uses fixed ports, so it can interfere with other processes.
You can use port "0" which automatically selects free port.
connector.startListening returns actual port assigned for the connection.


Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com




From: Alex Menkov 
To: Andrew Leonard , 
serviceability-dev@openjdk.java.net

Date: 07/06/2019 22:19
Subject: Re: RFR JDK-8225474: JDI connector accept fails "Address 
already in use" with concurrent listeners





Hi Andrew,

The fix looks good in general.
And I can be the sponsor.
I've tested the fix and got 1 test failure (of 400 runs) on Win-x64 and
the log looks the same.

java.lang.RuntimeException: Failed to accept connector connection
at JdwpConcurrentAttachTest.main(JdwpConcurrentAttachTest.java:74)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
Method)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:567)
at
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
at java.base/java.lang.Thread.run(Thread.java:830)
Caused by: java.net.BindException: Address already in use: bind
at java.base/sun.nio.ch.Net.bind0(Native Method)
at java.base/sun.nio.ch.Net.bind(Net.java:465)
at java.base/sun.nio.ch.Net.bind(Net.java:457)
at java.base/sun.nio.ch.NioSocketImpl.bind(NioSocketImpl.java:643)
at java.base/java.net.ServerSocket.bind(ServerSocket.java:359)
at java.base/java.net.ServerSocket.bind(ServerSocket.java:313)
at
jdk.jdi/com.sun.tools.jdi.SocketTransportService.startListening(SocketTransportService.java:300)
at
jdk.jdi/com.sun.tools.jdi.SocketTransportService.startListening(SocketTransportService.java:310)
at
jdk.jdi/com.sun.tools.jdi.GenericListeningConnector.startListening(GenericListeningConnector.java:117)
at
jdk.jdi/com.sun.tools.jdi.SocketListeningConnector.startListening(SocketListeningConnector.java:86)
at JdwpConcurrentAttachTest.attachTest(JdwpConcurrentAttachTest.java:111)
at JdwpConcurrentAttachTest.run(JdwpConcurrentAttachTest.java:93)

So looks like the fix doesn't fix the issue completely.

--alex

On 06/07/2019 07:24, Andrew Leonard wrote:

Hi,
Please can I request a sponsor and review of this patch for JDK-8225474.
A problem that we have seen intermittently with JDI connector stress
tests for quite a while that is caused by an non-thread safe HashMap  in
the connector class. I've created a standalone testcase that reproduces
it reliably.
http://cr.openjdk.java.net/~aleonard/8225474/webrev.00

Thanks
Andrew


Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com

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





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

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

[Robert Field]

Thanks Andrew!

If you want more data points on failure modes, this and the linked 
issues might be a rich source:


https://bugs.openjdk.java.net/browse/JDK-8184445

JShell is built on JDI and these various forms of intermittent attach 
failures have been a continual pain point.


-Robert


On 6/10/19 1:30 AM, Andrew Leonard wrote:

Thanks for the feedback everyone.

Hi Alan, yes I can put some cycles into digging deeper with this one, 
as you point out there's probably more that needs doing.
As Alex found running the new testcase 400 times it failed once still, 
interestingly during initial startListening() rather than the accept() 
where it was failing before.
On a general issue of "thread-safety", I had a good look through the 
spec docs and architecture docs and couldn't find any specification of 
thread-safety(or not), should it be inferred if the docs don't 
specifically state "thread-safe" that it should be assumed as 
"not-thread safe"?


Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com




From: Alan Bateman 
To: "serguei.spit...@oracle.com" , Andrew 
Leonard , 
serviceability-dev@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.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

Re: 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_leon...@uk.ibm.com 




From:   Alan Bateman 
To: "serguei.spit...@oracle.com" , Andrew 
Leonard , serviceability-dev@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.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

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

[Andrew Leonard]

Hi Alex,
Thanks for trying this out, it's possible this failure is an issue with my 
testcase, as I repeat the 10 thread test loop 10 times and I have a 30 
second timeout which potentially might overrun if a connection takes time 
to terminate. I could avoid that simply by using different ports for each 
run, equally it could also be an issue as pointed out by Alan.
Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 




From:   Alex Menkov 
To: Andrew Leonard , 
serviceability-dev@openjdk.java.net
Date:   07/06/2019 22:19
Subject:    Re: RFR JDK-8225474: JDI connector accept fails "Address 
already in use" with concurrent listeners



Hi Andrew,

The fix looks good in general.
And I can be the sponsor.
I've tested the fix and got 1 test failure (of 400 runs) on Win-x64 and 
the log looks the same.

java.lang.RuntimeException: Failed to accept connector connection
 at 
JdwpConcurrentAttachTest.main(JdwpConcurrentAttachTest.java:74)
 at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native 
Method)
 at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
 at 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
 at 
java.base/java.lang.reflect.Method.invoke(Method.java:567)
 at 
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
 at java.base/java.lang.Thread.run(Thread.java:830)
Caused by: java.net.BindException: Address already in use: bind
 at java.base/sun.nio.ch.Net.bind0(Native Method)
 at java.base/sun.nio.ch.Net.bind(Net.java:465)
 at java.base/sun.nio.ch.Net.bind(Net.java:457)
 at 
java.base/sun.nio.ch.NioSocketImpl.bind(NioSocketImpl.java:643)
 at 
java.base/java.net.ServerSocket.bind(ServerSocket.java:359)
 at 
java.base/java.net.ServerSocket.bind(ServerSocket.java:313)
 at 
jdk.jdi/com.sun.tools.jdi.SocketTransportService.startListening(SocketTransportService.java:300)
 at 
jdk.jdi/com.sun.tools.jdi.SocketTransportService.startListening(SocketTransportService.java:310)
 at 
jdk.jdi/com.sun.tools.jdi.GenericListeningConnector.startListening(GenericListeningConnector.java:117)
 at 
jdk.jdi/com.sun.tools.jdi.SocketListeningConnector.startListening(SocketListeningConnector.java:86)
 at 
JdwpConcurrentAttachTest.attachTest(JdwpConcurrentAttachTest.java:111)
 at 
JdwpConcurrentAttachTest.run(JdwpConcurrentAttachTest.java:93)

So looks like the fix doesn't fix the issue completely.

--alex

On 06/07/2019 07:24, Andrew Leonard wrote:
> Hi,
> Please can I request a sponsor and review of this patch for JDK-8225474. 

> A problem that we have seen intermittently with JDI connector stress 
> tests for quite a while that is caused by an non-thread safe HashMap in 
> the connector class. I've created a standalone testcase that reproduces 
> it reliably.
> 
https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Ealeonard_8225474_webrev.00&d=DwIC-g&c=jf_iaSHvJObTbx-siA1ZOg&r=NaV8Iy8Ld-vjpXZFDdTbgGlRTghGHnwM75wUPd5_NUQ&m=oCeyoE4YSIdSTvnXT1XeIp6wDP4UdqGpI35isidNG1M&s=fjbdD-X1whCGO82knPWEkDwXbHh8oO1xnJnncogRPsQ&e=
 

> 
> Thanks
> Andrew
> 
> 
> Andrew Leonard
> Java Runtimes Development
> IBM Hursley
> IBM United Kingdom Ltd
> internet email: andrew_m_leon...@uk.ibm.com
> 
> 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




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

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

[Andrew Leonard]

Thanks for the feedback everyone.

Hi Alan, yes I can put some cycles into digging deeper with this one, as 
you point out there's probably more that needs doing.
As Alex found running the new testcase 400 times it failed once still, 
interestingly during initial startListening() rather than the accept() 
where it was failing before.
On a general issue of "thread-safety", I had a good look through the spec 
docs and architecture docs and couldn't find any specification of 
thread-safety(or not), should it be inferred if the docs don't 
specifically state "thread-safe" that it should be assumed as "not-thread 
safe"?

Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 




From:   Alan Bateman 
To: "serguei.spit...@oracle.com" , Andrew 
Leonard , serviceability-dev@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.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

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

[David Holmes]

+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

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

[Alan Bateman]

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

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

[serguei.spit...@oracle.com]

  
  
Hi Andrew,
  
  It looks good to me.
  Thank you for your investigation and taking care about this!
  
  Thanks,
  Serguei
  
  
  On 6/7/19 07:24, Andrew Leonard wrote:

Hi,
  
  Please can I request
a sponsor and review of this patch for JDK-8225474. A problem
that we have
seen intermittently with JDI connector stress tests for quite a
while that
is caused by an non-thread safe HashMap in the connector class.
I've created
a standalone testcase that reproduces it reliably.
  
   
  http://cr.openjdk.java.net/~aleonard/8225474/webrev.00
  
  
  Thanks
  
  Andrew
  
  
  
  Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 
  
  
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
  


  

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

[Alex Menkov]

Hi Andrew,

The fix looks good in general.
And I can be the sponsor.
I've tested the fix and got 1 test failure (of 400 runs) on Win-x64 and 
the log looks the same.


java.lang.RuntimeException: Failed to accept connector connection
at JdwpConcurrentAttachTest.main(JdwpConcurrentAttachTest.java:74)
	at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native 
Method)
	at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

at java.base/java.lang.reflect.Method.invoke(Method.java:567)
	at 
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)

at java.base/java.lang.Thread.run(Thread.java:830)
Caused by: java.net.BindException: Address already in use: bind
at java.base/sun.nio.ch.Net.bind0(Native Method)
at java.base/sun.nio.ch.Net.bind(Net.java:465)
at java.base/sun.nio.ch.Net.bind(Net.java:457)
at java.base/sun.nio.ch.NioSocketImpl.bind(NioSocketImpl.java:643)
at java.base/java.net.ServerSocket.bind(ServerSocket.java:359)
at java.base/java.net.ServerSocket.bind(ServerSocket.java:313)
	at 
jdk.jdi/com.sun.tools.jdi.SocketTransportService.startListening(SocketTransportService.java:300)
	at 
jdk.jdi/com.sun.tools.jdi.SocketTransportService.startListening(SocketTransportService.java:310)
	at 
jdk.jdi/com.sun.tools.jdi.GenericListeningConnector.startListening(GenericListeningConnector.java:117)
	at 
jdk.jdi/com.sun.tools.jdi.SocketListeningConnector.startListening(SocketListeningConnector.java:86)

at 
JdwpConcurrentAttachTest.attachTest(JdwpConcurrentAttachTest.java:111)
at JdwpConcurrentAttachTest.run(JdwpConcurrentAttachTest.java:93)

So looks like the fix doesn't fix the issue completely.

--alex

On 06/07/2019 07:24, Andrew Leonard wrote:

Hi,
Please can I request a sponsor and review of this patch for JDK-8225474. 
A problem that we have seen intermittently with JDI connector stress 
tests for quite a while that is caused by an non-thread safe HashMap in 
the connector class. I've created a standalone testcase that reproduces 
it reliably.

http://cr.openjdk.java.net/~aleonard/8225474/webrev.00

Thanks
Andrew


Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com

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

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

[Robert Field]

Thanks Andrew!

Can I add my support for this.  I imagine this could be the cause of 
JShell failures in high-concurrency testing (JShell uses JDI, the 
failures are in connection).


This is not a review, as I haven't been on the JPDA team for a LONG 
time. Just a request that this be prioritized.


Thanks,
Robert


On 6/7/19 7:24 AM, Andrew Leonard wrote:

Hi,
Please can I request a sponsor and review of this patch for 
JDK-8225474. A problem that we have seen intermittently with JDI 
connector stress tests for quite a while that is caused by an 
non-thread safe HashMap in the connector class. I've created a 
standalone testcase that reproduces it reliably.

http://cr.openjdk.java.net/~aleonard/8225474/webrev.00

Thanks
Andrew


Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com

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

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

[Andrew Leonard]

Hi,
Please can I request a sponsor and review of this patch for JDK-8225474. A 
problem that we have seen intermittently with JDI connector stress tests 
for quite a while that is caused by an non-thread safe HashMap in the 
connector class. I've created a standalone testcase that reproduces it 
reliably.
http://cr.openjdk.java.net/~aleonard/8225474/webrev.00

Thanks
Andrew


Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 

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