This is the mail archive of the
java-patches@gcc.gnu.org
mailing list for the Java project.
Re: PlainSocketImpl changes to support socket timeouts (again)
- To: "Nic Ferrier" <nferrier at tapsellferrier dot co dot uk>
- Subject: Re: PlainSocketImpl changes to support socket timeouts (again)
- From: Tom Tromey <tromey at redhat dot com>
- Date: 30 Jul 2001 22:21:44 -0600
- Cc: java-patches at gcc dot gnu dot org, Alexandre Petit-Bianco <apbianco at cygnus dot com>
- References: <sb49bae2.072@tapsellferrier.co.uk>
- Reply-To: 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