Re: PlainSocketImpl changes to support socket timeouts (again)

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Nic" == Nic Ferrier <nferrier@tapsellferrier.co.uk> writes:

Nic> I've changed the patch for socket timeouts again. This fixed
Nic> version handles closing the socket in exactly the same way as
Nic> JDK's does (as far as I can tell by test anyway).

Hi Nic.

This patch is nearly ready to go in.  Just a few comments.

Nic> +  /** the socket's file handle.

First, thanks for adding javadoc.  Every little bit helps!

Nic> +  protected synchronized native void close () throws IOException;

There's no point in having `synchronized' and `native'.  In fact, I
think we should diagnose this as a warning (or maybe error), since it
is misleading -- the method won't synchronize automatically.

Nic> +  private native int read(byte[] buffer,int offset,int count) throws IOException;

Insert a newline before the `throws' (and indent the new line).
Lines over about 75 columns should be wrapped by hand.

Nic> +  private native void write(byte[] buffer,int offset,int count) throws IOException;

Likewise.

Nic> +  /** a cached copy of the in stream.
Nic> +   */
Nic> +  private InputStream in=null;

Use `in = null' -- the spaces are required by the coding standard.

Nic> +  private OutputStream out=null;

Likewise.

Nic> +    if(in==null)
Nic> +      in=new SocketInputStream(this);

Likewise (both lines).

There are more like this; I won't list them all.

Nic> +  //this can never happen - you can't call write on something that hasn't been created

The comment should start `// This' (space before the first word and
capital first letter) and should end with a `.'.  According to the
coding standard comments must be sentences (which generally I take to
mean in a lexical and not necessarily grammatical sense).  Also the
line should be wrapped so it is less than (about) 75 characters on a
line.

There are many comments like this that need tweaking.

Nic> +  throw new java::io::IOException ( JvNewStringLatin1 ("Socket writes unimplemented") );

This line is fine -- it doesn't have to be wrapped since there is no
sensible (according to our style) place to wrap it.

Nic> +  /***
Nic> +      FIXME!
Nic> +      This is the wierdest bug - if you leave this here
Nic> +      the ::close() will fail because fnum will have been
Nic> +      set to -1
Nic> +      Comment out this line and everything is fine.
Nic> +  //reset the socket fd
Nic> +  fnum = -1;

Maybe a compiler bug?  That's what it sounds like.

Nic> +/*
Nic> +  we don't use non-blocking mode in any of these write methods... 
Nic> +  presumably the socket is open already so won't block.
Nic> +  There's a potential issue here though - would it be a good idea to have
Nic> +  a socket IO thread that handled writes? 
Nic> +  We need some tests to find out how much write blocking occurs.
Nic> +
Nic> +  Nic Ferrier - June 2001
Nic> +*/

In general don't put your name in explanatory comments.
People can get that info from `cvs annotate' if they need it.

Nic> +  // FIXME: loop if r != 1.

I fixed this bug in natFileDescriptorPosix.cc.
I think the fixes should be carried into this code.


[ this one is out of order since I noticed it later, sorry ]
Nic> +    PlainSocketImpl socket;
Nic> +    
Nic> +    SocketInputStream(PlainSocketImpl socket)

A non-static inner class will have an implicit reference to its
creating object.  So there is no need to have `socket' as a field or
as an argument to the constructor.

Then you can replace calls to `socket.foo ()' with
`PlainSocketImpl.this.foo ()'.

Please make this change in both SocketInputStream and
SocketOutputStream.

Nic> +      socket.write(buffer,offset,length);

You need a space after each `,'.
This occurs in several places in the patch.


Nic> 2001-07-05  Nic Ferrier  <nferrier@tf1.tapsellferrier.co.uk>
Nic> 	* natPlainSocketImpl.cc: added timeout handling for sockets.
Nic> 	(close): new function closes the socket
Nic> 	(write): new functions for output to socket
Nic> 	(read): new functions for reading from socket
Nic> 	* PlainSocketImpl.java: glue for new timeout implementation
Nic> 	(write): calls the native impl
Nic> 	(read): likewise
Nic> 	(getInputStream): gets a stream to read from the socket
Nic> 	(getOutputStream): gets a stream to write to the socket

Use the path name for entries: `* java/net/natPlainSocketImpl.cc'.
Also, the first word of each description must be uppercase and the
description must end with a period.  Even, eg, `Likewise.'.


I don't mean to pick so many nits.  Your code is fine in essence.
However, the coding style is important to us; I've regretted letting
in patches that don't conform.

Thanks for doing this work.  It is something we've wanted for a long
time, but that nobody has been interested enough to do.  It's an
important contribution.

Tom