Re: Socket timeout patch

[Bryce McKinlay <bryce at waitaki dot otago dot ac dot nz>]

Nic Ferrier wrote:

>Enclosed within this message is the diff for the socket timeout
>patch. This has been a long time coming for such a small patch, 
>
Thanks for working on this Nic, its something that libgcj has been 
lacking for a long time.

>I hope that everyone will be happy with the formatting!
>
Actually I have found a few nits to pick ;-)

>
>This is the changelog entry for the patch:
>
>2001-12-14  Nic Ferrier  <nferrier@tf1.tapsellferrier.co.uk>
>
>	* natPlainSocketImpl.cc: added timeout handling for sockets.
>	(close): new function closes the socket
>

ChangeLog entries (like comments) should be punctuated as full sentences.

>
>+ *
>+ * @author Per Bothner <bothner@cygnus.com>
>+ * @date February 22, 1999.
>+ * @author Nic Ferrier <nferrier@tapsellferrier.co.uk>
>+ * @date December 15, 2001
>  */
>

Here you should just delete the dates (but keep the @author tags) - 
people can check the changelog if they need to find a date something was 
done.

>
>-
> class PlainSocketImpl extends SocketImpl
> {
>   // These fields are mirrored for use in native code to avoid cpp conflicts
>@@ -35,6 +35,18 @@
>                    _Jv_SO_SNDBUF_ = SocketOptions.SO_SNDBUF,
>                    _Jv_SO_RCVBUF_ = SocketOptions.SO_RCVBUF;
> 
>+  /** the socket's file handle.
>+   * This is the socket handle that is returned from the creation
>

First line of comment is redundant, and the Javadoc comments should 
always start on the second line (there are several places where this 
should be fixed in your patch).

>
>+  // Stream handling.
>+
>+  /** a cached copy of the in stream.
>+   */
>+  private InputStream in=null;
>+
>+  /** a cached copy of the out stream.
>+   */
>+  private OutputStream out=null;
>

Javadocs need to start on second line and start with capital letter. 
Also, the in=null is formatted wrong (should be "in = null"), but in 
this case the assignments are redundant and should be removed 
(initializing to zero or null is not neccessary in Java, and it causes 
GCJ to generate redundant finit code).

>
>-    if (in == null)
>+    if(in == null)
>
Wrong formatting.

>
>+  /** input stream which reads from the socket implementation.
>+   *
>+   * @author Nic Ferrier, nferrier@tapsellferrier.co.uk
>+   */
>+  class SocketInputStream
>+    extends InputStream
>+  {
>+    SocketInputStream()
>+    {
>+    }
>+    
>+    public void close() throws IOException
>+    {
>+      PlainSocketImpl.this.close();
>+    }
>+
>+    public int available() throws IOException
>+    {
>+      return PlainSocketImpl.this.available();
>+    }
>


Why were these implemented by delegating back to private natives in the 
SocketImpl rather than making them native themselves? It would be more 
efficient and reduce the method count.

>+  while (r != 1)
>+    {
>+      r = ::write (fnum, &d, 1);
>
Won't this loop forever in the case of an error that doesn't cause 
IOException?

>
>+	  // Some errors should not cause exceptions.
>+	  if (errno != ENOTCONN && errno != ECONNRESET && errno != EBADF)
>+	    throw new java::io::IOException (JvNewStringUTF (strerror (errno)));
>

Hmm, are you sure that ECONNRESET shouldn't throw? Also maybe it would 
be better to check for fnum
== 0 explicitly for the EBADF case?


>+  else if (r == -1)
>+    {
>+      if (errno == ECONNRESET)
>+	throw new java::io::IOException (JvNewStringUTF (strerror (errno)));
>+      // Some errors cause us to return end of stream...
>+      if (errno == ENOTCONN)
>+	return -1;
>+      // Other errors need to be signalled.
>+      throw new java::io::IOException (JvNewStringUTF (strerror (errno)));
>

Redundant ECONNRESET check?

regards

Bryce.