[RFA/JDWP] JdwpConnection

[Keith Seitz]

On Mon, 2005-06-06 at 16:40 -0600, Tom Tromey wrote:

> 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")'.

Yes, the spec specifically says that the handshake is ASCII. I've taken
your suggestion and forced the encoding to 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.

I've left this alone for now (see above). If you'd like me to change it
to something else, just let me know.

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

Forget the input side of things -- you're right: that does nothing. It
isn't necessary because only this thread will be reading the input
stream. (My first draft had things organized a little differently. I
forgot to whack the unnecessary synchronizations on the input.)

I don't know about the output side, though. I synchronized on the output
stream because I feared that two threads might try accessing the
transport (socket) at the same time. Reading through classpath's
java.io.DataOutputStream, I see that DataOutputStream.write (byte[],
int, int) is synchronized, so this isn't necessary after all. Am I
reading that correctly?

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

Fixed. I should have done this in the first place.

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

I didn't think it was necessary to worry about synchronization here. If
run executes one more loop, it won't really matter that much. Again,
you're call. If you want me to synchronize it, I can add that.

I don't think we need worry about checking if we're shutting down, but
it wouldn't hurt if we did, so I've added that.

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

Fixed.

I've attached the revised patch with the fixes I've noted above.

Thanks for looking at this! This file is a whole lot simpler now. :-)

Keith
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jdwp-connection.patch
Type: text/x-patch
Size: 7997 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/java-patches/attachments/20050607/dcb78dfa/attachment.bin>