RFR 8170541: serviceability/jdwp/AllModulesCommandTest.java fails intermittently on Windows and Solaris (original) (raw)

David Holmes david.holmes at oracle.com
Fri Mar 2 02:07:34 UTC 2018

Hi Daniil,

On 2/03/2018 11:53 AM, daniil.x.titov at oracle.com wrote:

Hi David,

Could you please say are you OK with the answers provided or there is something else that needs to be clarified?

Sorry. Yes the answers are fine - thanks.

David

Thanks!

Best regards, Daniil On 2/26/18 3:00 PM, daniil.x.titov at oracle.com wrote:

On 2/26/18 12:16 PM, Chris Plummer wrote: On 2/26/18 11:51 AM, daniil.x.titov at oracle.com wrote: Hi David and Sergei,

On 2/20/18 10:16 PM, serguei.spitsyn at oracle.com wrote: Hi David,

On 2/20/18 20:02, David Holmes wrote: Hi Daniil, Good find on this! What does the actual spec say about the length of things and how they may be split across multiple packets? Are we guaranteed that at most two packets will be involved? The JDWP spec (https://docs.oracle.com/javase/9/docs/specs/jdwp/jdwp-spec.html) says nothing about splitting JDWP reply packets at all but the implementation limits the max number of the sent packets to two packets max. The implementation is dated back to the initial load that happened in 2007 and the information about the related Jira issue is missing. open/src/jdk.jdwp.agent/share/native/libdtsocket/socketTransport.c 836    data = packet->type.cmd.data; 837    /* Do one send for short packets, two for longer ones */ 838    if (datalen <= MAXDATASIZE) {_ _839        memcpy(header + JDWPHEADERSIZE, data, datalen);_ _840        if (sendfully(socketFD, (char *)&header,_ _JDWPHEADERSIZE + datalen) !=_ _841            JDWPHEADERSIZE + datalen) {_ _842            RETURNIOERROR("send failed");_ _843        }_ _844    } else {_ _845        memcpy(header + JDWPHEADERSIZE, data, MAXDATASIZE);_ _846        if (sendfully(socketFD, (char *)&header,_ _JDWPHEADERSIZE + MAXDATASIZE) !=_ _847            JDWPHEADERSIZE + MAXDATASIZE) {_ _848            RETURNIOERROR("send failed");_ _849        }_ _850        /* Send the remaining data bytes right out of the data_ _area. */_ _851        if (sendfully(socketFD, (char *)data + MAXDATASIZE,_ _852                       datalen - MAXDATASIZE) != datalen -_ _MAXDATASIZE) {_ _853            RETURNIOERROR("send failed");_ _854        }_ _855    }_ _Curious. First packet is limited to MAXDATASIZE, 2nd packet has no_ _size limit. What's the point then of splitting it then? Is there a_ _desire to get the header transmitted in a smaller packet._ _Chris_ _It looks as the goal was to somehow improve the responsiveness in case_ _of the large data but I am not sure about this. I could not locate any_ _traces in Jira related to this implementation._ _Probably Serguei has some info what is the history behind this design._  _68     protected byte[] readJdwpString(DataInputStream ds) throws_ _IOException {_ _69         byte[] str = null;_ _70         int len = ds.readInt();_ _71         if (len > 0) { 72             str = new byte[len]; 73             ds.read(str, 0, len); 74         } might we get a short-read of the string if it is split across multiple packets? This and all other reads happen not directly from the socket input stream but rather from the  DataInputStream object that is constructed in  JdwpReply.initFromStream(InputStream) method. With the proposed fix we do ensure that the created DataInputStream object contains data from both packets in cases when the reply was split in two packets. Nice catch! Even though this fix is enough to resolve this problem now, there is a chance, it can fail in the future when more modules are added to the platform.

I'm wondering if all these reads should be loops, ensuring we read the expected amount of data. Since the implementation of the socket transport limits the max number of packets the reply might be split in to two packets I don't think we really need it here. One further comment - not sure why we need the print out for when we do read multiple packets? That would seem to be a debugging aid. Yes, it helps to understand what happens. Many tests have a lack of tracing which makes it harder to debug and understand failures. That is correct.  This additional tracing was added to help to understand the possible failures in the future. Thanks, Serguei

Thanks, David Thanks, Daniil On 21/02/2018 10:14 AM, Daniil Titov wrote: Hi Serguei, A new version of the webrev that has these strings reformatted is at http://cr.openjdk.java.net/~dtitov/8170541/webrev.02/ Thank you! Best regards, Daniil *From: *"serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com> *Date: *Tuesday, February 20, 2018 at 3:00 PM *To: *Daniil Titov <daniil.x.titov at oracle.com>, "serviceability-dev at openjdk.java.net" <serviceability-dev at openjdk.java.net> *Subject: *Re: RFR 8170541: serviceability/jdwp/AllModulesCommandTest.java fails intermittently on Windows and Solaris Hi Daniil, Interesting issue... Thank you for finding to the root cause so quickly! The fix looks good. Could I ask you to reformat these lines to make the L54 shorter ?: 54                 System.out.println("[" + getClass().getName() + "] Only " + bytesRead + " bytes of " + dataLength + 55                         " were read in the first packet. Reading the rest..."); Thanks, Serguei

On 2/20/18 09:24, Daniil Titov wrote: Please review the changes that fix intermittent failure of serviceability/jdwp/AllModulesCommandTest.java test. The problem here is that for a large data the JDWP agent (socketTransportwritePacket() method in src/jdk.jdwp.agent/share/native/libdtsocket/socketTransport.c ) sends 2 packets and in some cases only the first packet is received at the time when the test reads the reply from the JDWP agent. Since the test does not check that all data is received in the first packet the correlation between commands and replies became broken (the unread second packet is read by the next command and the reply for the next command is read by the next after next command and so on). Bug: https://bugs.openjdk.java.net/browse/JDK-8170541 Webrev: http://cr.openjdk.java.net/~dtitov/8170541/webrev.01 The tests ran successfully with Mach5. Best regards, Daniil

More information about the serviceability-dev mailing list