Hi David and Sergei,
On 2/20/18 10:16 PM, serguei.spit...@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/libdt_socket/socketTransport.c
836 data = packet->type.cmd.data;
837 /* Do one send for short packets, two for longer ones */
838 if (data_len <= MAX_DATA_SIZE) {
839 memcpy(header + JDWP_HEADER_SIZE, data, data_len);
840 if (send_fully(socketFD, (char *)&header, JDWP_HEADER_SIZE +
data_len) !=
841 JDWP_HEADER_SIZE + data_len) {
842 RETURN_IO_ERROR("send failed");
843 }
844 } else {
845 memcpy(header + JDWP_HEADER_SIZE, data, MAX_DATA_SIZE);
846 if (send_fully(socketFD, (char *)&header, JDWP_HEADER_SIZE +
MAX_DATA_SIZE) !=
847 JDWP_HEADER_SIZE + MAX_DATA_SIZE) {
848 RETURN_IO_ERROR("send failed");
849 }
850 /* Send the remaining data bytes right out of the data area. */
851 if (send_fully(socketFD, (char *)data + MAX_DATA_SIZE,
852 data_len - MAX_DATA_SIZE) != data_len -
MAX_DATA_SIZE) {
853 RETURN_IO_ERROR("send failed");
854 }
855 }
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?
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.spit...@oracle.com" <serguei.spit...@oracle.com>
*Date: *Tuesday, February 20, 2018 at 3:00 PM
*To: *Daniil Titov <daniil.x.ti...@oracle.com>,
"serviceability-dev@openjdk.java.net"
<serviceability-dev@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
