RFR 8170541: serviceability/jdwp/AllModulesCommandTest.java fails intermittently on Windows and Solaris

[serguei.spitsyn at oracle.com]

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? What about for other things eg:
>
>  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?

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.
>
> 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.


Thanks,
Serguei

>
> Thanks,
> David
>
> 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
>>     (socketTransport_writePacket() method in
>> src/jdk.jdwp.agent/share/native/libdt_socket/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
>>
>>
>>