Re: RFR JDK-8225474: JDI connector accept fails "Address already in use" with concurrent listeners
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
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
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&
Re: RFR JDK-8225474: JDI connector accept fails "Address already in use" with concurrent listeners
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.
ProcessAttachingConnector:
-com.sun.tools.attach.VirtualMachine vm;
final Transport transport;
==> Removed "unused" vm field
==> Made transport final to allow "safe publication"
public Transport transport() {
-if (transport == null) {
-return new Transport() {
-public String name() {
-return "local";
-}
-};
-}
return transport;
==> Removed creation by this method, as transport is always
constructed in constructor and is now final. This now matches transport()
method in all the other connector impls
SocketAttaching/ListeningConnector:
public SocketAttaching/ListeningConnector() {
-super(new SocketTransportService());
+super(new SocketTransportService(), new Transport() {
+ public String name() {
+ return "dt_socket"; //
for compatibility reasons
+ }
+});
==> Construct Transport() during super constructor where transport
is now "final" to ensure "safe publication", and avoids wasteful double
construction that was happening before.
SocketTransportService:
static class SocketListenKey extends ListenKey {
final ServerSocket ss;
==> Made "final" so that ListenKey returned by startListening() is
safe for publication, eg.if one thread starts listening and passes to
another thread to accept connection...
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
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
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
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
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_=DwIDaQ=jf_iaSHvJObTbx-siA1ZOg=NaV8Iy8Ld-vjpXZFDdTbgGlRTghGHnwM75wUPd5_NUQ=p--c3Hawio2kCsD0eo4leL8TLWZPtlYui8rmDo0E7So=EnJJuj3Yg8BeU7c2Fn6cqEeJGa6C5e4pIWGESAjwfhc=
>> 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)
>
Re: RFR JDK-8225474: JDI connector accept fails "Address already in use" with concurrent listeners
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 reproduces
>> it reliably.
>> http://cr.openjdk.java.net/~aleonard/
Re: RFR JDK-8225474: JDI connector accept fails "Address already in use" with concurrent listeners
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
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
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=DwIC-g=jf_iaSHvJObTbx-siA1ZOg=NaV8Iy8Ld-vjpXZFDdTbgGlRTghGHnwM75wUPd5_NUQ=OaT9vhOgj2gp0DQBwJgmItAjmzP-GsUEkGJAVbN9ZCE=EQwT-4TnAIqImG1-KTOT803z1DRjmTTeMtNV-9S9-6M= >> >> 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
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
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=DwIC-g=jf_iaSHvJObTbx-siA1ZOg=NaV8Iy8Ld-vjpXZFDdTbgGlRTghGHnwM75wUPd5_NUQ=oCeyoE4YSIdSTvnXT1XeIp6wDP4UdqGpI35isidNG1M=fjbdD-X1whCGO82knPWEkDwXbHh8oO1xnJnncogRPsQ= > > 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
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
RFR JDK-8225474: JDI connector accept fails "Address already in use" with concurrent listeners
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: 8201409: JDWP debugger initialization hangs intermittently
Hi Serguei, I've done some debug, and there seems to be some difference in behaviour with Hotspot with regards the report VM_INIT command, i'm not sure I quite understand why... so i'm consulting with a colleague to get some help. I'll update the bug when I have some more information. Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From: "serguei.spit...@oracle.com" <serguei.spit...@oracle.com> To: Andrew Leonard <andrew_m_leon...@uk.ibm.com> Cc: serviceability-dev@openjdk.java.net Date: 24/04/2018 01:47 Subject:Re: RFR: 8201409: JDWP debugger initialization hangs intermittently The most relevant stack traces from Solaris pstack dump are: 12 - lwp# 2 / thread# 2 13 7eae9b3c lwp_cond_wait (100123448, 100123430, 0, 0) 14 fffd12331a94 void os::PlatformEvent::park() (100123400, e6c40, e6c00, 0, fffd12adbf00, 100123430) + c4 15 fffd122c6038 int Monitor::IWait(Thread*,long) (100122f30, 100122000, 0, fffd12b4dba8, 0, 100122f50) + 98 16 fffd122c6a4c bool Monitor::wait(bool,long,bool) (100122f30, 1, 0, 0, 2155, 100122000) + 7c 17 fffd125464e8 int JavaThread::java_suspend_self() (100122000, 1, 4000, 1001220b8, 100122f30, 100122f30) + a8 18 fffd120c84a4 int JvmtiRawMonitor::raw_wait(long,bool,Thread*) (10079e3b0, 10079e430, 1, 100122000, 94400, fffd12b70600) + 254 19 fffd120942ec jvmtiError JvmtiEnv::RawMonitorWait(JvmtiRawMonitor*,long) (4, 10079e3b0, , 100122000, 0, fffd12adbf00) + 5c 20 fffd0f8385bc debugMonitorWait (fffd0f946268, c10, c00, fffd0f945580, fffd0f945580, 0) + 3c 21 fffd0f825140 enqueueCommand (1007a0a20, ffefc418, 103800, ffefc3e8, 0, fffd0f945580) + 140 22 fffd0f826e58 eventHelper_reportEvents (0, 10079a000, 0, 1007a0a20, 1, 1) + 108 23 fffd0f81b4c0 initialize (1001222e0, c00, 4, fffd0f83edc0, fffd0f9460d0, fffd0f945580) + 540 24 fffd0f81a77c cbEarlyException (fffd0f946190, 1001222e0, 1005953a0, fffd0f83eaa0, fffd0f946268, 1005953a8) + 21c 25 fffd120af6a4 void JvmtiExport::post_exception_throw(JavaThread*,Method*,unsigned char*,oopDesc*) (100122000, fffd12b78f38, 100764b40, 62cb91b50, 100122d48, fffd12b7a618) + 15b4 26 fffd11c27484 unsigned char*InterpreterRuntime::exception_handler_for_exception(JavaThread*,oopDesc*) (0, 4a0014e0, 100122000, 4a001470, 100122d40, fffd12adbf00) + 704 27 6100eac0 * JITDebug.debugTarget()V+29 28 61009ecc * JITDebug.parseArgs([Ljava/lang/String;)Z+50 29 610084ac * JITDebug.main([Ljava/lang/String;)V+8 30 610003c0 (7d0ffc20, 7d0ffe30, a, 4a000f18, 6100ff40, 7d0ffd78) + fff8006cc46de0b0 31 fffd11c43134 void JavaCalls::call_helper(JavaValue*,const methodHandle&,JavaCallArguments*,Thread*) (7d0ffe28, 4a000f18, 7d0ffd68, 7d0ffde8, e, 61000340) + 2e4 32 fffd11cfd95c jni_CallStaticVoidMethod (1001222e0, 1400, 1c, 7d0ffde8, 100122000, 153d) + 5bc 33 fffd12c0603c JavaMain (ac8, 100123780, 100123798, 1, fffd12b605f0, fffd11cfd3a0) + 81c 34 7eae4b38 _lwp_start (0, 0, 0, 0, 0, 0) 216 - lwp# 18 / thread# 18 217 7eae9b3c lwp_cond_wait (10078cd48, 10078cd30, 0, 0) 218 fffd12331a94 void os::PlatformEvent::park() (10078cd00, e6c40, e6c00, 0, fffd12adbf00, 10078cd30) + c4 219 fffd120c7b00 int JvmtiRawMonitor::SimpleWait(Thread*,long) (10078a470, 10079c000, , fffd120c50e0, 1, 0) + 100 220 fffd120c8328 int JvmtiRawMonitor::raw_wait(long,bool,Thread*) (10078a470, , 1, 10079c000, ffd880ac, fffd12adbf00) + d8 221 fffd120942ec jvmtiError JvmtiEnv::RawMonitorWait(JvmtiRawMonitor*,long) (4, 10078a470, , 10079c000, 0, fffd12adbf00) + 5c 222 fffd0f8385bc debugMonitorWait (fffd0f946268, c10, c00, fffd0f945580, fffd0f946190, 0) + 3c 223 fffd0f81adac debugInit_waitVMInitComplete (0, f28, fffd0f945580, c00, fffd0f844888, 1) + 3c 224 fffd0f81caac debugLoop_run (105c00, f50, fffd0f945580, 1, fffd0f9464d0, 0) + 6c 225 fffd0f8335b4 connectionInitiated (fffd0f705250, 1000, 1, fffd0f945580, fffd0f9468d8, 0) + e4 226 fffd0f833800 acceptThread (0, fffd0f705260, 1007973c0, fffd0f843de0, fffd0f945580, fffd0f946190) + 110 227 fffd120c0934 void JvmtiAgentThread::call_start_function() (10079c000, 0, 0, fffd12adbf00, fffd12b78f38, 0) + 1b4 228 fffd125445fc void JavaThread::thread_main_inner() (
Re: RFR: 8201409: JDWP debugger initialization hangs intermittently
Hi Serguei, Good find, i'll try it out and do some debugging. Many thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From: "serguei.spit...@oracle.com" <serguei.spit...@oracle.com> To: Andrew Leonard <andrew_m_leon...@uk.ibm.com> Cc: serviceability-dev@openjdk.java.net Date: 24/04/2018 01:03 Subject:Re: RFR: 8201409: JDWP debugger initialization hangs intermittently On 4/23/18 16:08, serguei.spit...@oracle.com wrote: Hi Andrew, There is a regression with this fix. The following test is failed with timeout on all platforms except Windows: Sorry, forgot to copy the test name: open/test/jdk/com/sun/jdi/JITDebug.sh Thanks, Serguei I'll try to get more details about this timeout. Thanks, Serguei On 4/18/18 09:49, serguei.spit...@oracle.com wrote: Hi Andrew, Sorry, I did not reply earlier. The fix need more testing. We also have some important tests in closed. I'll run them but I'm a little bit busy at the moment. You have two reviews which is enough for push after testing. Thanks, Serguei On 4/18/18 08:23, Andrew Leonard wrote: Hi Serguei, Do you need me to try anything else for this review? hotspot/jtreg/serviceability suite run successfully. Many Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From:"serguei.spit...@oracle.com" <serguei.spit...@oracle.com> To: daniel.daughe...@oracle.com, Andrew Leonard <andrew_m_leon...@uk.ibm.com> Cc:serviceability-dev@openjdk.java.net Date:16/04/2018 07:10 Subject:Re: RFR: 8201409: JDWP debugger initialization hangs intermittently On 4/15/18 10:01, Daniel D. Daugherty wrote: On 4/13/18 3:07 PM, serguei.spit...@oracle.com wrote: Andrew and reviewers, I'm re-sending this RFR with a corrected subject that includes the bug number. The issues is: https://bugs.openjdk.java.net/browse/JDK-8201409 Webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8201409-jdwp-initsync.ibm.1/ src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c No comments. src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h No comments. src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c So now pauses in debugLoop_run() before the loop that reads cmds. Looks good. src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c So the VM_INIT event handler now signals that we have received the VM_INIT event so that allows debugLoop_run() to proceed. Serguei, this fix needs to have the most of the Serviceability stack of tests run against it (jdwp, JVM/TI, JDI and jdb tests). Based on the email thread, I can't tell which tests have been run with the fix in place. Hi Dan, I'm going to sponsor this fix and will run all the debugger tests. Sorry that I did not announce it yet. Thanks, Serguei Dan The fix looks good to me. Also, I've agreed to skip a unit test as creating it for this issue is not easy. At least, one more review is needed before the fix can be pushed. Thanks, Serguei On 4/11/18 06:33, Andrew Leonard wrote: Hi Serguei, Thank you for raising the bug. I had a chat with one of my colleagues who could recreate it, and it's probably related to the handshaking that is done in the particular scenario. So with the JCK harness: com.sun.jck.lib.ExecJCKTestOtherJVMCmd LD_LIBRARY_PATH=/javatest/lib/jck /jck8b/natives/linux_x86-64 /projects/jck/jdwp/j2sdk-image/bin/java -Xdump:system:none -Xdump:system:events=gpf+abort+traceassert+corruptcache -Xdump:snap:none -Xdump:snap:events=gpf+abort+traceassert+corruptcache -Xdump:java:none -Xdump:java:events=gpf+abort+traceassert+corruptcache -Xdump:heap:none -Xdump:heap:events=gpf+abort+traceassert+corruptcache - Xfuture -agentlib:jdwp=server=y,transport=dt_socket,address=localhost :35000,suspend=y -classpath /javatest/lib/jck /JCK8b-b03/JCK-runtime-8b/classes -Djava.security.policy=/javatest/lib/jck /JCK8b-b03/JCK-runtime-8b/lib/jck.policy javasoft.sqe.jck.lib.jpda.jdwp.DebuggeeLoader -waittime=600 -msgSwitch=ub1604x64vm10:38636 -componentName= ArrayReference.GetValues.getvalues002 Note that the JCK test harness starts the target process, attaches to it, and sends the resume command in a very short time with no handshaking. That may not help..but hopefully helps explain things a bit? It's the timing of the resume command during the test that is crucial, resuming before the VM initialization is complete will trigger it. Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From:"serguei.spit...@oracle.com" <serguei.spit...@oracle.com>
Re: RFR: 8201409: JDWP debugger initialization hangs intermittently
No Problem, thanks Serguei, let me know if I can help in any way. Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From: "serguei.spit...@oracle.com" <serguei.spit...@oracle.com> To: Andrew Leonard <andrew_m_leon...@uk.ibm.com> Cc: daniel.daughe...@oracle.com, serviceability-dev@openjdk.java.net Date: 18/04/2018 17:56 Subject:Re: RFR: 8201409: JDWP debugger initialization hangs intermittently Hi Andrew, Sorry, I did not reply earlier. The fix need more testing. We also have some important tests in closed. I'll run them but I'm a little bit busy at the moment. You have two reviews which is enough for push after testing. Thanks, Serguei On 4/18/18 08:23, Andrew Leonard wrote: Hi Serguei, Do you need me to try anything else for this review? hotspot/jtreg/serviceability suite run successfully. Many Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From:"serguei.spit...@oracle.com" <serguei.spit...@oracle.com> To: daniel.daughe...@oracle.com, Andrew Leonard <andrew_m_leon...@uk.ibm.com> Cc:serviceability-dev@openjdk.java.net Date:16/04/2018 07:10 Subject:Re: RFR: 8201409: JDWP debugger initialization hangs intermittently On 4/15/18 10:01, Daniel D. Daugherty wrote: On 4/13/18 3:07 PM, serguei.spit...@oracle.com wrote: Andrew and reviewers, I'm re-sending this RFR with a corrected subject that includes the bug number. The issues is: https://bugs.openjdk.java.net/browse/JDK-8201409 Webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8201409-jdwp-initsync.ibm.1/ src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c No comments. src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h No comments. src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c So now pauses in debugLoop_run() before the loop that reads cmds. Looks good. src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c So the VM_INIT event handler now signals that we have received the VM_INIT event so that allows debugLoop_run() to proceed. Serguei, this fix needs to have the most of the Serviceability stack of tests run against it (jdwp, JVM/TI, JDI and jdb tests). Based on the email thread, I can't tell which tests have been run with the fix in place. Hi Dan, I'm going to sponsor this fix and will run all the debugger tests. Sorry that I did not announce it yet. Thanks, Serguei Dan The fix looks good to me. Also, I've agreed to skip a unit test as creating it for this issue is not easy. At least, one more review is needed before the fix can be pushed. Thanks, Serguei On 4/11/18 06:33, Andrew Leonard wrote: Hi Serguei, Thank you for raising the bug. I had a chat with one of my colleagues who could recreate it, and it's probably related to the handshaking that is done in the particular scenario. So with the JCK harness: com.sun.jck.lib.ExecJCKTestOtherJVMCmd LD_LIBRARY_PATH=/javatest/lib/jck /jck8b/natives/linux_x86-64 /projects/jck/jdwp/j2sdk-image/bin/java -Xdump:system:none -Xdump:system:events=gpf+abort+traceassert+corruptcache -Xdump:snap:none -Xdump:snap:events=gpf+abort+traceassert+corruptcache -Xdump:java:none -Xdump:java:events=gpf+abort+traceassert+corruptcache -Xdump:heap:none -Xdump:heap:events=gpf+abort+traceassert+corruptcache - Xfuture -agentlib:jdwp=server=y,transport=dt_socket,address=localhost :35000,suspend=y -classpath /javatest/lib/jck /JCK8b-b03/JCK-runtime-8b/classes -Djava.security.policy=/javatest/lib/jck /JCK8b-b03/JCK-runtime-8b/lib/jck.policy javasoft.sqe.jck.lib.jpda.jdwp.DebuggeeLoader -waittime=600 -msgSwitch=ub1604x64vm10:38636 -componentName= ArrayReference.GetValues.getvalues002 Note that the JCK test harness starts the target process, attaches to it, and sends the resume command in a very short time with no handshaking. That may not help..but hopefully helps explain things a bit? It's the timing of the resume command during the test that is crucial, resuming before the VM initialization is complete will trigger it. Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From:"serguei.spit...@oracle.com" <serguei.spit...@oracle.com> To:Andrew Leonard <andrew_m_leon...@uk.ibm.com> Cc:serviceability-dev@openjdk.java.net Date:11/04/2018 09:57 Subject:Re: RFR: Fix race condition in jdwp Hi Andrew, I've filed the bug: https://bugs.openjdk.java.net/browse/JDK-8201409 Also, this is a webrev with your patch: http://cr.openjdk.java.net/~sspitsyn
Re: RFR: 8201409: JDWP debugger initialization hangs intermittently
Hi Serguei, Do you need me to try anything else for this review? hotspot/jtreg/serviceability suite run successfully. Many Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From: "serguei.spit...@oracle.com" <serguei.spit...@oracle.com> To: daniel.daughe...@oracle.com, Andrew Leonard <andrew_m_leon...@uk.ibm.com> Cc: serviceability-dev@openjdk.java.net Date: 16/04/2018 07:10 Subject:Re: RFR: 8201409: JDWP debugger initialization hangs intermittently On 4/15/18 10:01, Daniel D. Daugherty wrote: On 4/13/18 3:07 PM, serguei.spit...@oracle.com wrote: Andrew and reviewers, I'm re-sending this RFR with a corrected subject that includes the bug number. The issues is: https://bugs.openjdk.java.net/browse/JDK-8201409 Webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8201409-jdwp-initsync.ibm.1/ src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c No comments. src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h No comments. src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c So now pauses in debugLoop_run() before the loop that reads cmds. Looks good. src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c So the VM_INIT event handler now signals that we have received the VM_INIT event so that allows debugLoop_run() to proceed. Serguei, this fix needs to have the most of the Serviceability stack of tests run against it (jdwp, JVM/TI, JDI and jdb tests). Based on the email thread, I can't tell which tests have been run with the fix in place. Hi Dan, I'm going to sponsor this fix and will run all the debugger tests. Sorry that I did not announce it yet. Thanks, Serguei Dan The fix looks good to me. Also, I've agreed to skip a unit test as creating it for this issue is not easy. At least, one more review is needed before the fix can be pushed. Thanks, Serguei On 4/11/18 06:33, Andrew Leonard wrote: Hi Serguei, Thank you for raising the bug. I had a chat with one of my colleagues who could recreate it, and it's probably related to the handshaking that is done in the particular scenario. So with the JCK harness: com.sun.jck.lib.ExecJCKTestOtherJVMCmd LD_LIBRARY_PATH=/javatest/lib/jck /jck8b/natives/linux_x86-64 /projects/jck/jdwp/j2sdk-image/bin/java -Xdump:system:none -Xdump:system:events=gpf+abort+traceassert+corruptcache -Xdump:snap:none -Xdump:snap:events=gpf+abort+traceassert+corruptcache -Xdump:java:none -Xdump:java:events=gpf+abort+traceassert+corruptcache -Xdump:heap:none -Xdump:heap:events=gpf+abort+traceassert+corruptcache - Xfuture -agentlib:jdwp=server=y,transport=dt_socket,address=localhost :35000,suspend=y -classpath /javatest/lib/jck /JCK8b-b03/JCK-runtime-8b/classes -Djava.security.policy=/javatest/lib/jck /JCK8b-b03/JCK-runtime-8b/lib/jck.policy javasoft.sqe.jck.lib.jpda.jdwp.DebuggeeLoader -waittime=600 -msgSwitch=ub1604x64vm10:38636 -componentName= ArrayReference.GetValues.getvalues002 Note that the JCK test harness starts the target process, attaches to it, and sends the resume command in a very short time with no handshaking. That may not help..but hopefully helps explain things a bit? It's the timing of the resume command during the test that is crucial, resuming before the VM initialization is complete will trigger it. Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From:"serguei.spit...@oracle.com" <serguei.spit...@oracle.com> To:Andrew Leonard <andrew_m_leon...@uk.ibm.com> Cc:serviceability-dev@openjdk.java.net Date:11/04/2018 09:57 Subject:Re: RFR: Fix race condition in jdwp Hi Andrew, I've filed the bug: https://bugs.openjdk.java.net/browse/JDK-8201409 Also, this is a webrev with your patch: http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8201409-jdwp-initsync.ibm.1/ I agree that creating a standalone test is tricky here. I've added usleep(1) into the eventHelper_reportVMInit() and ran the JTreg com/sun/jdi tests with my JDK build. However, none of the tests failed with the failure mode you described. So that I'm puzzled a little bit. I suspect that some specific debugLoop commands were used in your scenario. It is still possible that I've missed something here. Will try to double check everything. Thanks, Serguei On 4/11/18 01:29, Andrew Leonard wrote: Thanks Serguei, I terms of a standalone testcase it is quite tricky, as due to the nature of the issue which took a lot of investigation to solve it's very timing dependent and will only occur randomly. It can be forced as I indicated below by adding a "sleep" in the VMInit report code but that's not a testcase, however the issue
Re: RFR: 8201409: JDWP debugger initialization hangs intermittently
Hi Daniel, Thanks for reviewing this. Just to let you know, I have successfully run all the jdk_core and hotspot/jtreg/serviceability tests with this patch in place. Cheers Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From: "Daniel D. Daugherty" <daniel.daughe...@oracle.com> To: "serguei.spit...@oracle.com" <serguei.spit...@oracle.com>, Andrew Leonard <andrew_m_leon...@uk.ibm.com> Cc: serviceability-dev@openjdk.java.net Date: 15/04/2018 18:00 Subject:Re: RFR: 8201409: JDWP debugger initialization hangs intermittently On 4/13/18 3:07 PM, serguei.spit...@oracle.com wrote: Andrew and reviewers, I'm re-sending this RFR with a corrected subject that includes the bug number. The issues is: https://bugs.openjdk.java.net/browse/JDK-8201409 Webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8201409-jdwp-initsync.ibm.1/ src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c No comments. src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h No comments. src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c So now pauses in debugLoop_run() before the loop that reads cmds. Looks good. src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c So the VM_INIT event handler now signals that we have received the VM_INIT event so that allows debugLoop_run() to proceed. Serguei, this fix needs to have the most of the Serviceability stack of tests run against it (jdwp, JVM/TI, JDI and jdb tests). Based on the email thread, I can't tell which tests have been run with the fix in place. Dan The fix looks good to me. Also, I've agreed to skip a unit test as creating it for this issue is not easy. At least, one more review is needed before the fix can be pushed. Thanks, Serguei On 4/11/18 06:33, Andrew Leonard wrote: Hi Serguei, Thank you for raising the bug. I had a chat with one of my colleagues who could recreate it, and it's probably related to the handshaking that is done in the particular scenario. So with the JCK harness: com.sun.jck.lib.ExecJCKTestOtherJVMCmd LD_LIBRARY_PATH=/javatest/lib/jck /jck8b/natives/linux_x86-64 /projects/jck/jdwp/j2sdk-image/bin/java -Xdump:system:none -Xdump:system:events=gpf+abort+traceassert+corruptcache -Xdump:snap:none -Xdump:snap:events=gpf+abort+traceassert+corruptcache -Xdump:java:none -Xdump:java:events=gpf+abort+traceassert+corruptcache -Xdump:heap:none -Xdump:heap:events=gpf+abort+traceassert+corruptcache - Xfuture -agentlib:jdwp=server=y,transport=dt_socket,address=localhost :35000,suspend=y -classpath /javatest/lib/jck /JCK8b-b03/JCK-runtime-8b/classes -Djava.security.policy=/javatest/lib/jck /JCK8b-b03/JCK-runtime-8b/lib/jck.policy javasoft.sqe.jck.lib.jpda.jdwp.DebuggeeLoader -waittime=600 -msgSwitch=ub1604x64vm10:38636 -componentName= ArrayReference.GetValues.getvalues002 Note that the JCK test harness starts the target process, attaches to it, and sends the resume command in a very short time with no handshaking. That may not help..but hopefully helps explain things a bit? It's the timing of the resume command during the test that is crucial, resuming before the VM initialization is complete will trigger it. Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From:"serguei.spit...@oracle.com" <serguei.spit...@oracle.com> To:Andrew Leonard <andrew_m_leon...@uk.ibm.com> Cc:serviceability-dev@openjdk.java.net Date:11/04/2018 09:57 Subject:Re: RFR: Fix race condition in jdwp Hi Andrew, I've filed the bug: https://bugs.openjdk.java.net/browse/JDK-8201409 Also, this is a webrev with your patch: http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8201409-jdwp-initsync.ibm.1/ I agree that creating a standalone test is tricky here. I've added usleep(1) into the eventHelper_reportVMInit() and ran the JTreg com/sun/jdi tests with my JDK build. However, none of the tests failed with the failure mode you described. So that I'm puzzled a little bit. I suspect that some specific debugLoop commands were used in your scenario. It is still possible that I've missed something here. Will try to double check everything. Thanks, Serguei On 4/11/18 01:29, Andrew Leonard wrote: Thanks Serguei, I terms of a standalone testcase it is quite tricky, as due to the nature of the issue which took a lot of investigation to solve it's very timing dependent and will only occur randomly. It can be forced as I indicated below by adding a "sleep" in the VMInit report code but that's not a testcase, however the issue was originally found in our JCK testing for IBMJava8, testcase test.jck8b.runtime.vm.jdwp, but again
Re: RFR: Fix race condition in jdwp
Hi Serguei, Thank you for raising the bug. I had a chat with one of my colleagues who could recreate it, and it's probably related to the handshaking that is done in the particular scenario. So with the JCK harness: com.sun.jck.lib.ExecJCKTestOtherJVMCmd LD_LIBRARY_PATH=/javatest/lib/jck /jck8b/natives/linux_x86-64 /projects/jck/jdwp/j2sdk-image/bin/java -Xdump:system:none -Xdump:system:events=gpf+abort+traceassert+corruptcache -Xdump:snap:none -Xdump:snap:events=gpf+abort+traceassert+corruptcache -Xdump:java:none -Xdump:java:events=gpf+abort+traceassert+corruptcache -Xdump:heap:none -Xdump:heap:events=gpf+abort+traceassert+corruptcache - Xfuture -agentlib:jdwp=server=y,transport=dt_socket,address=localhost :35000,suspend=y -classpath /javatest/lib/jck /JCK8b-b03/JCK-runtime-8b/classes -Djava.security.policy=/javatest/lib/jck /JCK8b-b03/JCK-runtime-8b/lib/jck.policy javasoft.sqe.jck.lib.jpda.jdwp.DebuggeeLoader -waittime=600 -msgSwitch=ub1604x64vm10:38636 -componentName= ArrayReference.GetValues.getvalues002 Note that the JCK test harness starts the target process, attaches to it, and sends the resume command in a very short time with no handshaking. That may not help..but hopefully helps explain things a bit? It's the timing of the resume command during the test that is crucial, resuming before the VM initialization is complete will trigger it. Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From: "serguei.spit...@oracle.com" <serguei.spit...@oracle.com> To: Andrew Leonard <andrew_m_leon...@uk.ibm.com> Cc: serviceability-dev@openjdk.java.net Date: 11/04/2018 09:57 Subject:Re: RFR: Fix race condition in jdwp Hi Andrew, I've filed the bug: https://bugs.openjdk.java.net/browse/JDK-8201409 Also, this is a webrev with your patch: http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8201409-jdwp-initsync.ibm.1/ I agree that creating a standalone test is tricky here. I've added usleep(1) into the eventHelper_reportVMInit() and ran the JTreg com/sun/jdi tests with my JDK build. However, none of the tests failed with the failure mode you described. So that I'm puzzled a little bit. I suspect that some specific debugLoop commands were used in your scenario. It is still possible that I've missed something here. Will try to double check everything. Thanks, Serguei On 4/11/18 01:29, Andrew Leonard wrote: Thanks Serguei, I terms of a standalone testcase it is quite tricky, as due to the nature of the issue which took a lot of investigation to solve it's very timing dependent and will only occur randomly. It can be forced as I indicated below by adding a "sleep" in the VMInit report code but that's not a testcase, however the issue was originally found in our JCK testing for IBMJava8, testcase test.jck8b.runtime.vm.jdwp, but again only happened intermittently. Sort of like "performance" type issues we're not always going to be able to create a testcase that will always "fail" if the fix is not present. Your thoughts? Cheers Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From:"serguei.spit...@oracle.com" <serguei.spit...@oracle.com> To:Andrew Leonard <andrew_m_leon...@uk.ibm.com> Cc:serviceability-dev@openjdk.java.net Date:11/04/2018 01:02 Subject:Re: RFR: Fix race condition in jdwp Hi Andrew, Okay, I'll file a bug on this topic. But do you have a standalone test demonstrating this issue? Thanks, Serguei On 4/10/18 06:23, Andrew Leonard wrote: Hi Serguei, I don't have access to the bug database to raise one, are you able to please? Summary: JDWP debugger initialization hangs intermittently Description: If during the JDWP setup initialization the VM initialization takes slightly longer than the main debug initialization thread a "hang" situation can occur. This has been seen in testcase test.jck8b.runtime.vm.jdwp and can also be recreated easily by adding a 10 second sleep to the beginning of the src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c method eventHelper_reportVMInit() . First seen: JDK8 Recreated: JDK11 Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From: "serguei.spit...@oracle.com" <serguei.spit...@oracle.com> To:Andrew Leonard <andrew_m_leon...@uk.ibm.com>, serviceability-dev@openjdk.java.net Date:09/04/2018 23:03 Subject:Re: RFR: Fix race condition in jdwp Hi Andrew, The patch itself looks reasonable. However, in ord
Re: RFR: Fix race condition in jdwp
Hi Serguei,
I don't have access to the bug database to raise one, are you able to
please?
Summary: JDWP debugger initialization hangs intermittently
Description: If during the JDWP setup initialization the VM initialization
takes slightly longer than the main debug initialization thread a "hang"
situation can occur. This has been seen in testcase
test.jck8b.runtime.vm.jdwp and can also be recreated easily by adding a 10
second sleep to the beginning of the
src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c method
eventHelper_reportVMInit() .
First seen: JDK8
Recreated: JDK11
Thanks
Andrew
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com
From: "serguei.spit...@oracle.com" <serguei.spit...@oracle.com>
To: Andrew Leonard <andrew_m_leon...@uk.ibm.com>,
serviceability-dev@openjdk.java.net
Date: 09/04/2018 23:03
Subject:Re: RFR: Fix race condition in jdwp
Hi Andrew,
The patch itself looks reasonable.
However, in order to proceed with it, a bug report with a standalone
test case demonstrating the issue is needed.
Thanks,
Serguei
On 4/9/18 09:07, Andrew Leonard wrote:
> Hi,
> We discovered in our testing with OpenJ9 that a race condition can
> occur in the jdwp under certain circumstances, and we were able to
> force the same issue with Hotspot. Normally, the event helper thread
> suspends all threads, then the debug loop in the listener thread
> receives a command to resume. The debugger may deadlock if the debug
> loop in the listener thread starts processing commands (e.g. resume
> threads) before the event helper completes the initialization (and
> suspends threads).
>
> This patch adds synchronization to ensure the event helper completes
> the initialization sequence before debugger commands are processed.
>
> Please can I find a sponsor for this contribution? Patch below..
>
> Many thanks
>
> Andrew
>
>
>
> diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
> b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
> --- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
> +++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights
> reserved.
> + * Copyright (c) 1998, 2018, Oracle and/or its affiliates. All rights
> reserved.
> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> *
> * This code is free software; you can redistribute it and/or modify it
> @@ -58,6 +58,7 @@
> static jboolean vmInitialized;
> static jrawMonitorID initMonitor;
> static jboolean initComplete;
> +static jboolean VMInitComplete;
> static jbyte currentSessionID;
>
> /*
> @@ -617,6 +618,35 @@
> debugMonitorExit(initMonitor);
> }
>
> +/*
> + * Signal VM initialization is complete.
> + */
> +void
> +signalVMInitComplete(void)
> +{
> +/*
> + * VM Initialization is complete
> + */
> +LOG_MISC(("signal VM initialization complete"));
> +debugMonitorEnter(initMonitor);
> +VMInitComplete = JNI_TRUE;
> +debugMonitorNotifyAll(initMonitor);
> +debugMonitorExit(initMonitor);
> +}
> +
> +/*
> + * Wait for VM initialization to complete.
> + */
> +void
> +debugInit_waitVMInitComplete(void)
> +{
> +debugMonitorEnter(initMonitor);
> +while (!VMInitComplete) {
> +debugMonitorWait(initMonitor);
> +}
> +debugMonitorExit(initMonitor);
> +}
> +
> /* All process exit() calls come from here */
> void
> forceExit(int exit_code)
> @@ -672,6 +702,7 @@
> LOG_MISC(("Begin initialize()"));
> currentSessionID = 0;
> initComplete = JNI_FALSE;
> +VMInitComplete = JNI_FALSE;
>
> if ( gdata->vmDead ) {
> EXIT_ERROR(AGENT_ERROR_INTERNAL,"VM dead at initialize() time");
> diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
> b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
> --- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
> +++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 1998, 2015, Oracle and/or its affiliates. All rights
> reserved.
> + * Copyright (c) 1998, 2018, Oracle and/or its affiliates. All rights
> reserved.
> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> *
> * This code is free software; you can redistribute it and/or modify it
> @@ -39,4 +39,7 @@
> void debugInit_exit(jvmtiError, const char *);
> void forceExit(int);
>
> +void debugInit_waitVMInitComplete(void);
> +void signalVMInitComplete(void);
> +
>
Re: RFR: Fix race condition in jdwp
Hi David,
The existing "initComplete" in debugInit.c logic is the debug main thread
initialization complete logic, that does not handle nor needs to wait for
the asynchronous VM initialization that will be reported but must be
completed before the debug loop can start processing cmds.
Thanks
Andrew
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com
From: David Holmes <david.hol...@oracle.com>
To: Andrew Leonard <andrew_m_leon...@uk.ibm.com>,
serviceability-dev@openjdk.java.net
Date: 10/04/2018 02:06
Subject:Re: RFR: Fix race condition in jdwp
Hi Andrew,
On 10/04/2018 2:07 AM, Andrew Leonard wrote:
> Hi,
> We discovered in our testing with OpenJ9 that a race condition can occur
> in the jdwp under certain circumstances, and we were able to force the
> same issue with Hotspot. Normally, the event helper thread suspends all
> threads, then the debug loop in the listener thread receives a command
> to resume. The debugger may deadlock if the debug loop in the listener
> thread starts processing commands (e.g. resume threads) before the event
> helper completes the initialization (and suspends threads).
>
> This patch adds synchronization to ensure the event helper completes the
> initialization sequence before debugger commands are processed.
How does this relate to the existing debugInit_waitInitComplete? I don't
see why we would want two initialization synchronization mechanisms. ??
Thanks,
David
> Please can I find a sponsor for this contribution? Patch below..
>
> Many thanks
>
> Andrew
>
>
>
> diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
> b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
> --- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
> +++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights
> reserved.
> + * Copyright (c) 1998, 2018, Oracle and/or its affiliates. All rights
> reserved.
>* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>*
>* This code is free software; you can redistribute it and/or modify
it
> @@ -58,6 +58,7 @@
> static jboolean vmInitialized;
> static jrawMonitorID initMonitor;
> static jboolean initComplete;
> +static jboolean VMInitComplete;
> static jbyte currentSessionID;
>
> /*
> @@ -617,6 +618,35 @@
> debugMonitorExit(initMonitor);
> }
>
> +/*
> + * Signal VM initialization is complete.
> + */
> +void
> +signalVMInitComplete(void)
> +{
> +/*
> + * VM Initialization is complete
> + */
> +LOG_MISC(("signal VM initialization complete"));
> +debugMonitorEnter(initMonitor);
> +VMInitComplete = JNI_TRUE;
> +debugMonitorNotifyAll(initMonitor);
> +debugMonitorExit(initMonitor);
> +}
> +
> +/*
> + * Wait for VM initialization to complete.
> + */
> +void
> +debugInit_waitVMInitComplete(void)
> +{
> +debugMonitorEnter(initMonitor);
> +while (!VMInitComplete) {
> +debugMonitorWait(initMonitor);
> +}
> +debugMonitorExit(initMonitor);
> +}
> +
> /* All process exit() calls come from here */
> void
> forceExit(int exit_code)
> @@ -672,6 +702,7 @@
> LOG_MISC(("Begin initialize()"));
> currentSessionID = 0;
> initComplete = JNI_FALSE;
> +VMInitComplete = JNI_FALSE;
>
> if ( gdata->vmDead ) {
> EXIT_ERROR(AGENT_ERROR_INTERNAL,"VM dead at initialize() time");
> diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
> b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
> --- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
> +++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 1998, 2015, Oracle and/or its affiliates. All rights
> reserved.
> + * Copyright (c) 1998, 2018, Oracle and/or its affiliates. All rights
> reserved.
>* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>*
>* This code is free software; you can redistribute it and/or modify
it
> @@ -39,4 +39,7 @@
> void debugInit_exit(jvmtiError, const char *);
> void forceExit(int);
>
> +void debugInit_waitVMInitComplete(void);
> +void signalVMInitComplete(void);
> +
> #endif
> diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
> b/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
> --- a/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
> +++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
> @@ -1,5 +1,5
RFR: Fix race condition in jdwp
Hi,
We discovered in our testing with OpenJ9 that a race condition can occur
in the jdwp under certain circumstances, and we were able to force the
same issue with Hotspot. Normally, the event helper thread suspends all
threads, then the debug loop in the listener thread receives a command to
resume. The debugger may deadlock if the debug loop in the listener thread
starts processing commands (e.g. resume threads) before the event helper
completes the initialization (and suspends threads).
This patch adds synchronization to ensure the event helper completes the
initialization sequence before debugger commands are processed.
Please can I find a sponsor for this contribution? Patch below..
Many thanks
Andrew
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
--- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 1998, 2018, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -58,6 +58,7 @@
static jboolean vmInitialized;
static jrawMonitorID initMonitor;
static jboolean initComplete;
+static jboolean VMInitComplete;
static jbyte currentSessionID;
/*
@@ -617,6 +618,35 @@
debugMonitorExit(initMonitor);
}
+/*
+ * Signal VM initialization is complete.
+ */
+void
+signalVMInitComplete(void)
+{
+/*
+ * VM Initialization is complete
+ */
+LOG_MISC(("signal VM initialization complete"));
+debugMonitorEnter(initMonitor);
+VMInitComplete = JNI_TRUE;
+debugMonitorNotifyAll(initMonitor);
+debugMonitorExit(initMonitor);
+}
+
+/*
+ * Wait for VM initialization to complete.
+ */
+void
+debugInit_waitVMInitComplete(void)
+{
+debugMonitorEnter(initMonitor);
+while (!VMInitComplete) {
+debugMonitorWait(initMonitor);
+}
+debugMonitorExit(initMonitor);
+}
+
/* All process exit() calls come from here */
void
forceExit(int exit_code)
@@ -672,6 +702,7 @@
LOG_MISC(("Begin initialize()"));
currentSessionID = 0;
initComplete = JNI_FALSE;
+VMInitComplete = JNI_FALSE;
if ( gdata->vmDead ) {
EXIT_ERROR(AGENT_ERROR_INTERNAL,"VM dead at initialize() time");
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
--- a/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1998, 2015, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 1998, 2018, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -39,4 +39,7 @@
void debugInit_exit(jvmtiError, const char *);
void forceExit(int);
+void debugInit_waitVMInitComplete(void);
+void signalVMInitComplete(void);
+
#endif
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
b/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
--- a/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 1998, 2018, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -98,6 +98,7 @@
standardHandlers_onConnect();
threadControl_onConnect();
+debugInit_waitVMInitComplete();
/* Okay, start reading cmds! */
while (shouldListen) {
if (!dequeue()) {
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
b/src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
--- a/src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 1998, 2018, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -580,6 +580,7 @@
(void)threadControl_suspendThread(command->thread, JNI_FALSE);
}
+signalVMInitComplete();
outStream_initCommand(, uniqueID(), 0x0,
JDWP_COMMAND_SET(Event),
JDWP_COMMAND(Event, Composite));
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, e
Re: RFR (XS): 8198539: Cleanup of unused imports in java/util/jar/Attributes.java (java.base) and JdpController.java (jdk.management.agent)
looks good Cheers Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From: "Langer, Christoph" <christoph.lan...@sap.com> To: OpenJDK Dev list <core-libs-...@openjdk.java.net>, "serviceability-dev@openjdk.java.net" <serviceability-dev@openjdk.java.net> Cc: "andrew_m_leon...@uk.ibm.com" <andrew_m_leon...@uk.ibm.com> Date: 22/02/2018 08:42 Subject:RFR (XS): 8198539: Cleanup of unused imports in java/util/jar/Attributes.java (java.base) and JdpController.java (jdk.management.agent) Hi, please review a simple import cleanup fix for java/util/jar/Attributes.java and sun/management/jdp/JdpController.java. Bug: https://bugs.openjdk.java.net/browse/JDK-8198539 Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8198539.0/ Thanks and best regards Christoph 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: Remove unused import proposal: in JdpController.java
Thank you Christoph Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From: "Langer, Christoph" <christoph.lan...@sap.com> To: David Holmes <david.hol...@oracle.com>, Andrew Leonard <andrew_m_leon...@uk.ibm.com>, "serviceability-dev@openjdk.java.net" <serviceability-dev@openjdk.java.net> Date: 22/02/2018 08:26 Subject:RE: Remove unused import proposal: in JdpController.java Hi Andrew, I'll take this on as I have a similar include cleanup change in my queue for java/util/jar/Attributes.java. Will post a separate review request... Best regards > -Original Message- > From: serviceability-dev [mailto:serviceability-dev- > boun...@openjdk.java.net] On Behalf Of David Holmes > Sent: Donnerstag, 22. Februar 2018 05:12 > To: Andrew Leonard <andrew_m_leon...@uk.ibm.com>; serviceability- > d...@openjdk.java.net > Subject: Re: Remove unused import proposal: in JdpController.java > > Hi Andrew, > > I've filed: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8198539=DwIFAw=jf_iaSHvJObTbx-siA1ZOg=NaV8Iy8Ld-vjpXZFDdTbgGlRTghGHnwM75wUPd5_NUQ=ViGiFidumq45NMcWmohOp_xM59zUHYwEnA94bbvXpvA=tp8_Q0Fm7qSARlXkA1Qew-9-vQ_rXuKic8vN8-SXlf0= > > to clean up your leftover import from JDK-8183123 :) (which was fixed in > 10 not 9). > > If someone from serviceability doesn't pick this up I may be able to. > > David > > On 22/02/2018 1:29 AM, Andrew Leonard wrote: > > Hi, > > I would like to find a sponsor please for this simple tidy up of > > JdpController.java ? > > It contains an unused "import sun.management.VMManagement;" which > is for > > a sun specific java.management class which this class used to use prior > > to jdk9. > > Thanks > > Andrew > > > > diff --git > > > a/src/jdk.management.agent/share/classes/sun/management/jdp/JdpCont > roller.java > > > b/src/jdk.management.agent/share/classes/sun/management/jdp/JdpCont > roller.java > > > > --- > > > a/src/jdk.management.agent/share/classes/sun/management/jdp/JdpCont > roller.java > > > > +++ > > > b/src/jdk.management.agent/share/classes/sun/management/jdp/JdpCont > roller.java > > > > @@ -34,7 +34,6 @@ > > import java.lang.reflect.Field; > > import java.lang.reflect.Method; > > import java.lang.UnsupportedOperationException; > > -import sun.management.VMManagement; > > > > /** > >* JdpController is responsible to create and manage a broadcast loop. > > > > > > > > > > > > Andrew Leonard > > Java Runtimes Development > > IBM Hursley > > IBM United Kingdom Ltd > > Phone internal: 245913, external: 01962 815913 > > 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
Remove unused import proposal: in JdpController.java
Hi, I would like to find a sponsor please for this simple tidy up of JdpController.java ? It contains an unused "import sun.management.VMManagement;" which is for a sun specific java.management class which this class used to use prior to jdk9. Thanks Andrew diff --git a/src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java b/src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java --- a/src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java +++ b/src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java @@ -34,7 +34,6 @@ import java.lang.reflect.Field; import java.lang.reflect.Method; import java.lang.UnsupportedOperationException; -import sun.management.VMManagement; /** * JdpController is responsible to create and manage a broadcast loop. Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 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 8183123 : JDP packets have no processId context set
Hi Daniel, Thank you for the review. You actually make a good observation, I had intended to just change that method implementation to use the ProcessHandle class, however the pid() method returns a "long" and the containing method returns an Integer, so it needs "lossy" casting to an (int) before "boxing" to an Integer. Thinking again about this, given this is just a private method within this class that is only called from one place, it seems cleaner to change the private method to return a Long object, and change the calling instance appropriately. I also see if I look at the javadoc for ProcessHandle.pid() that it can in "theory" return UnsupportOperationException, so I have also handled that. I have a new webrev, which I will ask Christoph to upload... Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From: "Daniel D. Daugherty" <daniel.daughe...@oracle.com> To: Andrew Leonard <andrew_m_leon...@uk.ibm.com>, serviceability-dev@openjdk.java.net Date: 05/07/2017 21:27 Subject:Re: RFR 8183123 : JDP packets have no processId context set On 6/29/17 7:57 AM, Andrew Leonard wrote: Hi All, Please can I get some review feedback for my changes for this issue: https://bugs.openjdk.java.net/browse/JDK-8183123 The webrev patch has been uploaded here: http://cr.openjdk.java.net/~clanger/webrevs/8183123.0/ src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java L137 return (int)ProcessHandle.current().pid(); The return type is Integer. Why not cast to "Integer" instead of "int"? test/sun/management/jdp/JdpOnTestCase.java The test update just verifies that a non-NULL PROCESS_ID is found, but doesn't verify the format (integer) of the return. Of course, a platform independent format for PROCESS_ID might be problematic... For example, in some versions of Cygwin, I've seen negative values for PIDs... Thumbs up. If you change the cast I don't need to see a new webrev. Dan Essentially the fix entails: - Replacing invalid process id query logic with call to ProcessHandle.current().getPid(). - Update testcase to cover the failing scenario. Thus it fails without my patch, and succeeds with it. Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 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 8183123 : JDP packets have no processId context set
Thanks Christoph, Any offers for another reviewer of this fix please, it's only a few lines of code change. Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: andrew_m_leon...@uk.ibm.com From: "Langer, Christoph" <christoph.lan...@sap.com> To: "serviceability-dev@openjdk.java.net" <serviceability-dev@openjdk.java.net> Date: 03/07/2017 08:25 Subject:RE: RFR 8183123 : JDP packets have no processId context set Sent by:"serviceability-dev" <serviceability-dev-boun...@openjdk.java.net> Hi, Ping: Can we please get another review for this rather small JDK 10 fix? Thanks Christoph From: serviceability-dev [ mailto:serviceability-dev-boun...@openjdk.java.net] On Behalf Of Andrew Leonard Sent: Donnerstag, 29. Juni 2017 15:57 To: serviceability-dev@openjdk.java.net Subject: RFR 8183123 : JDP packets have no processId context set Hi All, Please can I get some review feedback for my changes for this issue: https://bugs.openjdk.java.net/browse/JDK-8183123 The webrev patch has been uploaded here: http://cr.openjdk.java.net/~clanger/webrevs/8183123.0/ Essentially the fix entails: - Replacing invalid process id query logic with call to ProcessHandle.current().getPid(). - Update testcase to cover the failing scenario. Thus it fails without my patch, and succeeds with it. Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 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
RFR 8183123 : JDP packets have no processId context set
Hi All, Please can I get some review feedback for my changes for this issue: https://bugs.openjdk.java.net/browse/JDK-8183123 The webrev patch has been uploaded here: http://cr.openjdk.java.net/~clanger/webrevs/8183123.0/ Essentially the fix entails: - Replacing invalid process id query logic with call to ProcessHandle.current().getPid(). - Update testcase to cover the failing scenario. Thus it fails without my patch, and succeeds with it. Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 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: Proposal:JdpController.getProcessId() VM compatibility improvement
Thanks Roger for taking a look, i'll see what the serviceability community
think,
cheers
Andrew
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com
From: Roger Riggs <roger.ri...@oracle.com>
To: Andrew Leonard <andrew_m_leon...@uk.ibm.com>
Cc: serviceability-dev@openjdk.java.net, Core-Libs-Dev
<core-libs-...@openjdk.java.net>
Date: 26/06/2017 19:59
Subject:Re: Proposal:JdpController.getProcessId() VM compatibility
improvement
Hi Andrew,
Redirecting back to serviceability-dev@openjdk.java.net, it is the better
list for this topic.
I don't know these tests but it seems odd to stick that assert into every
callback to packetFromThisVMReceived. Perhaps someone more familiar can
review and sponsor.
Thanks, Roger
On 6/26/2017 10:37 AM, Andrew Leonard wrote:
Hi Roger,
I have now updated to the latest openjdk9 and examined the existing jdp
tests. I have added to those tests to assert the JDP packet processId,
which was in fact "null", so it was failing to set it before... Hence,
with this testcase update they fail without my patch, and succeed with the
new patch.
Being new to this contribution process, where do I go from here please?
Thanks
Andrew
diff -r 2425838cfb5e
src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java
---
a/src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java
Fri Jun 23 14:32:59 2017 -0400
+++
b/src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java
Mon Jun 26 15:31:15 2017 +0100
@@ -133,20 +133,8 @@
// Get the process id of the current running Java process
private static Integer getProcessId() {
-try {
-// Get the current process id using a reflection hack
-RuntimeMXBean runtime = ManagementFactory.getRuntimeMXBean();
-Field jvm = runtime.getClass().getDeclaredField("jvm");
-jvm.setAccessible(true);
-
-VMManagement mgmt = (sun.management.VMManagement)
jvm.get(runtime);
-Method pid_method =
mgmt.getClass().getDeclaredMethod("getProcessId");
-pid_method.setAccessible(true);
-Integer pid = (Integer) pid_method.invoke(mgmt);
-return pid;
-} catch(Exception ex) {
-return null;
-}
+// Get the current process id
+return (int)ProcessHandle.current().pid();
}
diff -r 2425838cfb5e test/sun/management/jdp/JdpOnTestCase.java
--- a/test/sun/management/jdp/JdpOnTestCase.javaFri Jun 23
14:32:59 2017 -0400
+++ b/test/sun/management/jdp/JdpOnTestCase.javaMon Jun 26
15:30:51 2017 +0100
@@ -31,6 +31,7 @@
import java.net.SocketTimeoutException;
import java.util.Map;
+import static jdk.testlibrary.Asserts.assertNotEquals;
public class JdpOnTestCase extends JdpTestCase {
@@ -58,6 +59,7 @@
final String jdpName = payload.get("INSTANCE_NAME");
log.fine("Received correct JDP packet #" +
String.valueOf(receivedJDPpackets) +
", jdp.name=" + jdpName);
+assertNotEquals(null, payload.get("PROCESS_ID"), "Expected
payload.get(\"PROCESS_ID\") to be not null.");
}
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com
From:Roger Riggs <roger.ri...@oracle.com>
To:Andrew Leonard <andrew_m_leon...@uk.ibm.com>
Cc:core-libs-...@openjdk.java.net
Date:23/06/2017 13:57
Subject:Re: Proposal:JdpController.getProcessId() VM compatibility
improvement
Hi Leonard,
No need to stray from your intended focus.
The other is just another cleanup.
Thanks, Roger
On 6/23/17 6:57 AM, Andrew Leonard wrote:
Thanks Roger,
So yes that issue does look similar, although I was only looking in the
management interfaces. I'll have a peek at those references to see if they
are similar, it would make sense to clean those up too, to be consistent.
Your thoughts?
I'll also see if there's a junit for this method, if not i'll see if I can
add one.
Cheers
Andrew
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com
From:Roger Riggs <roger.ri...@oracle.com>
To:core-libs-...@openjdk.java.net
Date:22/06/2017 18:03
Subject:Re: Proposal:JdpController.getProcessId() VM compatibility
improvement
Sent by:"core-libs-dev" <core-libs-dev-boun...@openjdk.java.net>
HI,
This looks like:
JDK-8074569 <https://bugs.openjdk.java.net/browse/JDK-8
Fw: Proposal:JdpController.getProcessId() VM compatibility improvement
Hello,
I would like to propose the change below to the
JdpController.getProcessId() method which currently uses a reflection hack
and the VMManagement interface. Instead for JDK9+ we can use the
ProcessHandle.current().getPid() method, which will also allow better
compatibility with other pluggable VMs.
jdk/src/jdk.management.agent/share/classes/sun/management/jdp/JdpController.java
36d35
< import sun.management.VMManagement;
136,149c135,136
< try {
< // Get the current process id using a reflection hack
< RuntimeMXBean runtime =
ManagementFactory.getRuntimeMXBean();
< Field jvm = runtime.getClass().getDeclaredField("jvm");
< jvm.setAccessible(true);
<
< VMManagement mgmt = (sun.management.VMManagement)
jvm.get(runtime);
< Method pid_method =
mgmt.getClass().getDeclaredMethod("getProcessId");
< pid_method.setAccessible(true);
< Integer pid = (Integer) pid_method.invoke(mgmt);
< return pid;
< } catch(Exception ex) {
< return null;
< }
---
> // Get the current process id
> return (int)ProcessHandle.current().getPid();
I'd appreciate any feedback please, and help obtaining a suitable sponsor
and contributor?
Thanks
Andrew
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
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
