[RFA/JDWP] JdwpConnection

Tom Tromey tromey@redhat.com
Tue Jun 7 00:21:00 GMT 2005


>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> The spec for this command says that the VM be "abruptly" shut
Keith> down. I believe this means via System.exit -- the whole thing
Keith> (VM & back-end) dies.

System.exit actually can result in a fair amount of work being
done -- running finalizers, various other exit-time cleanups, etc.
Perhaps they mean something even lower level, like C's exit().

In addition to the things Hans said ...

Keith> +    // Wait for handshake from debugger
Keith> +    DataInputStream istream
Keith> +      = new DataInputStream (_transport.getInputStream ());
Keith> +    byte[] hshake = new byte[_HANDSHAKE.length ()];
Keith> +    istream.readFully (hshake, 0, _HANDSHAKE.length ());
Keith> +    String h = new String (hshake);

This String constructor uses the default encoding, which isn't
guaranteed to be anything in particular.  You probably want something
like 'new String(hshake, "ASCII")'.

Keith> +	DataOutputStream ostream
Keith> +	  = new DataOutputStream (_transport.getOutputStream ());
Keith> +	ostream.writeBytes (_HANDSHAKE);

I'm a little surprised that this method isn't deprecated, as it so
unicode-unfriendly.  In general it should be avoided but it is ok if
we know this handshake will only be ASCII.

Keith> +    DataInputStream stream;
Keith> +    synchronized (this)
Keith> +      {
Keith> +	stream = new DataInputStream (_transport.getInputStream ());
Keith> +      }
Keith> +
Keith> +    byte[] data = null;
Keith> +    synchronized (stream)
Keith> +      {

I think this synchronization isn't doing anything.
'stream' is allocated in this method and does not escape.  So, no
other thread can possibly hold this lock.

Maybe you want to wrap the transport's input stream in a
DataInputStream once and then reuse that?  I don't know.

I see the same thing, but for the write side, in sendPacket().

Keith> +  /**
Keith> +   * Shutdown the connection
Keith> +   */
Keith> +  public void shutdown ()
Keith> +  {
Keith> +    synchronized (this)
Keith> +      {
Keith> +	_transport.shutdown ();
Keith> +      }
Keith> +
Keith> +    _shutdown = true;
Keith> +    this.interrupt ();
Keith> +  }
Keith> +}

The synchronization here and in run() seems a little wrong.
run() tests '_shutdown' without synchronization.
shutdown() sets that field without synchronization (and doesn't test
to see if we've already shut down ... not sure if that matters).

Instead of 'this.interrupt()' you can just write 'interrupt()'.
(I don't mind either way actually, sometimes the 'this' is clearer.)

Tom



More information about the Java-patches mailing list