YA Socket timeout patch

[Nic Ferrier <nferrier at tf1 dot tapsellferrier dot co dot uk>]

Ok. Here I've tried to make some of the javadoc fit what you guys are
saying.

Personally, I think you should follow the convention for javadoc that
I use on the Paperclips project, because it's better and consistent
and it looks good.

I use the first line (the one with the double **) to be a very brief
intro, as per the Sun suggestions. This means you can right very
simple search tools to pull out doc intro's to things by scanning
lines for **.

It also makes clear that the rest of the comment, starting on the
second line, is a more detailed overview.


However, for the sake of harmony (and to get this damn patch off my
plate and get on with something else) I have altered some of the doc
comments to start on the second line.

The doc comments that are just a single line I have not altered. It
seems a waste to make a line for nothing.


Bryce said:
>>>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).
>>>
Nic replied:
>>Maybe, but it also makes clear what is going on. The variables are
>>cache variables, Someone reading the related methods might not
>>understand them.

Bryce came back:
>I agree that it makes the code clearer in some cases to explicitly 
>specify it, but it does cause GCJ to generate less efficient code. 
>Perhaps we can fix GCJ to remove/ignore the redundant assignment though. 
>Do other bytecode compilers generate field initializer code for null and 
>zero initializations?

I think we sorted this one. The value has to be initialized if you're
going to rely on it. I wouldn't change this even if it were not true:
not initializing goes against all my feelings about code readability
and correctness.


Bryce said:
>>>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.

Nic replied:
>>If they were themselves natives, wouldn't that mean an extra 2 classes?
>>(with associated C++ equivalents)

Bryce came back:
>No. Only the java compiler ever generates class objects - so there is 
>only one class object  even if the class has native methods. In this 
>case, since the SocketInput/OutputStream classes are inner classes 
>anyway, I see no reason to not just implement their native methods in 
>natPlainSocketImpl.cc also.

I looked at this, I couldn't get it to work. I tried just setting the
methods to native within the inner classes, GCJ wasn't able to match
the inner class methods to the methods declared in
natPlainSocketImpl.cc.

I didn't like the idea anyway... I don't think it's very clear what's
going on... So, instead of making them native I made them final. I
think they should be final anyway.


Bryce said:
>I'm sorry to be so pedantic, but it is important to try to keep the code 
>consistent and clean in order to preserve its long-term
maintainability.

Part of the reason I'm a bit cross about your pedandtry is that I have
had conversations about functionality with other people and agreed the
method that I would use... and then someone else comes along (in this
case, you) and says "oh, I don't like the way you've done that, or
this..." and it's very frustrating.

IMHO the important thing was to fix this annoying missing feature (one
might call it a bug) and not worry too much about the exact way in
which it was done as long as it wasn't grossly inefficient, which this
isn't.


Anyway, let me know if there are any objections. If not, I'll check it
in and we can all get on with our lives /8->


Here's the new changelog:

2001-07-05  Nic Ferrier  <nferrier@tf1.tapsellferrier.co.uk>

	* natPlainSocketImpl.cc: Added read timeout handling for sockets.
	(close): New function closes the socket.
	(write): New functions for output to the socket.
	(read): New functions for reading from the socket.
	* PlainSocketImpl.java: Added glue for new timeout implementation.
	(write): Calls the native implementation.
	(read): likewise.
	(getInputStream): Gets a stream to read from the socket.
	(getOutputStream): Gets a stream to write to the socket.


And here's the patch itself:


Index: PlainSocketImpl.java
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/net/PlainSocketImpl.java,v
retrieving revision 1.8
diff -u -r1.8 PlainSocketImpl.java
--- PlainSocketImpl.java	2000/12/08 10:28:32	1.8
+++ PlainSocketImpl.java	2001/12/16 15:12:12
@@ -11,17 +11,16 @@
 package java.net;
 import java.io.*;
 
-/**
- * @author Per Bothner <bothner@cygnus.com>
- * @date February 22, 1999.
- */
 
 /**
+ * The standard GCJ socket implementation.
  * Written using on-line Java Platform 1.2 API Specification, as well
  * as "The Java Class Libraries", 2nd edition (Addison-Wesley, 1998).
  * Status:  Believed complete and correct.
+ *
+ * @author Per Bothner <bothner@cygnus.com>
+ * @author Nic Ferrier <nferrier@tapsellferrier.co.uk>
  */
-
 class PlainSocketImpl extends SocketImpl
 {
   // These fields are mirrored for use in native code to avoid cpp conflicts
@@ -35,6 +34,18 @@
                    _Jv_SO_SNDBUF_ = SocketOptions.SO_SNDBUF,
                    _Jv_SO_RCVBUF_ = SocketOptions.SO_RCVBUF;
 
+  /**
+   * The OS file handle representing the socket.
+   * This is used for reads and writes to/from the socket and
+   * to close it.
+   *
+   * {@link SocketImpl#fd} is created from this like so:
+   * <pre>
+   *   fd = new FileDescriptor (fnum);
+   * </pre>
+   *
+   * When the socket is closed this is reset to -1.
+   */
   int fnum = -1;
 
   // This value is set/read by setOption/getOption.
@@ -62,37 +73,129 @@
   protected native void listen (int backlog) throws IOException;
 
   private native void accept (PlainSocketImpl s) throws IOException;
+
   protected void accept (SocketImpl s) throws IOException
   {
     accept((PlainSocketImpl) s);
   }
+
+  protected native int available() throws IOException;
+
+  protected native void close () throws IOException;
+
+
+  // Stream handling.
+
+  /** A cached copy of the in stream for reading from the socket.
+   */
+  private InputStream in = null;
+
+  /** A cached copy of the out stream for writing to the socket.
+   */
+  private OutputStream out = null;
 
-  private InputStream in;
-  private OutputStream out;
 
+  // The native read methods.
+
+  private native int read() throws IOException;
+
+  private native int read(byte[] buffer, int offset, int count)
+    throws IOException;
+
+
+  // The native write methods.
+
+  private native void write(int c) throws IOException;
+
+  private native void write(byte[] buffer, int offset, int count)
+    throws IOException;
+
+
+  /** @return the input stream attached to the socket.
+   */
   protected InputStream getInputStream() throws IOException
   {
-    // FIXME: TODO - Implement class SocketInputStream timeouts in read();
-    if (in == null)
-      in = new FileInputStream (fd);
+    if(in == null)
+      in = new SocketInputStream();
     return in;
   }
 
+  /** @return the output stream attached to the socket.
+   */
   protected OutputStream getOutputStream() throws IOException
   {
     if (out == null)
-      out = new FileOutputStream (fd);
+      out = new SocketOutputStream();
     return out;
   }
 
-  protected int available () throws IOException
-  {
-    return in.available();
+  /**
+   * A stream which reads from the socket implementation.
+   *
+   * @author Nic Ferrier <nferrier@tapsellferrier.co.uk>
+   */
+  class SocketInputStream
+    extends InputStream
+  {
+    SocketInputStream()
+    {
+    }
+    
+    public final void close() throws IOException
+    {
+      PlainSocketImpl.this.close();
+    }
+
+    public final int available() throws IOException
+    {
+      return PlainSocketImpl.this.available();
+    }
+
+    public final int read() throws IOException
+    {
+      return PlainSocketImpl.this.read();
+    }
+
+    public final int read(byte[] buffer, int offset, int length)
+      throws IOException
+    {
+      return PlainSocketImpl.this.read(buffer, offset, length);
+    }
+
+    public final int read(byte[] buffer)
+      throws IOException
+    {
+      return PlainSocketImpl.this.read(buffer, 0, buffer.length);
+    }
   }
 
-  protected void close () throws IOException
-  {
-    if (fd.valid())
-      fd.close();
+  /** A stream which writes to the socket implementation.
+   *
+   * @author Nic Ferrier  <nferrier@tapsellferrier.co.uk>
+   */
+  class SocketOutputStream
+    extends OutputStream
+  {
+    public final void close() throws IOException
+    {
+      PlainSocketImpl.this.close();
+    }
+
+    public final void write(int c) throws IOException
+    {
+      PlainSocketImpl.this.write(c);
+    }
+
+    public final void write(byte[] buffer, int offset, int length)
+      throws IOException
+    {
+      PlainSocketImpl.this.write(buffer, offset, length);
+    }
+
+    public final void write(byte[] buffer)
+      throws IOException
+    {
+      PlainSocketImpl.this.write(buffer, 0, buffer.length);
+    }
   }
 }
Index: natPlainSocketImpl.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/net/natPlainSocketImpl.cc,v
retrieving revision 1.25
diff -u -r1.25 natPlainSocketImpl.cc
--- natPlainSocketImpl.cc	2001/08/01 17:53:00	1.25
+++ natPlainSocketImpl.cc	2001/12/16 15:12:13
@@ -85,6 +85,9 @@
 #include <java/lang/Boolean.h>
 #include <java/lang/Class.h>
 #include <java/lang/Integer.h>
+#include <java/lang/Thread.h>
+#include <java/lang/NullPointerException.h>
+#include <java/lang/ArrayIndexOutOfBoundsException.h>
 
 #define BooleanClass java::lang::Boolean::class$
 
@@ -326,6 +329,258 @@
   char* strerr = strerror (errno);
   throw new java::io::IOException (JvNewStringUTF (strerr));
 }
+
+// Close(shutdown) the socket.
+void
+java::net::PlainSocketImpl::close()
+{
+  // should we use shutdown here? how would that effect so_linger?
+  int res = ::close (fnum);
+
+  if (res == -1)
+    {
+      // These three errors are not errors according to tests performed
+      // on the reference implementation.
+      if (errno != ENOTCONN && errno != ECONNRESET && errno != EBADF)
+	throw new java::io::IOException  (JvNewStringUTF (strerror (errno)));
+    }
+  // Safe place to reset the file pointer.
+  fnum = -1;
+}
+
+// Write a byte to the socket.
+void
+java::net::PlainSocketImpl::write(jint b)
+{
+  jbyte d =(jbyte) b;
+  int r = 0;
+
+  while (r != 1)
+    {
+      r = ::write (fnum, &d, 1);
+      if (r == -1)
+	{
+	  if (java::lang::Thread::interrupted())
+	    {
+	      java::io::InterruptedIOException *iioe
+		= new java::io::InterruptedIOException 
+		(JvNewStringLatin1 (strerror (errno)));
+	      iioe->bytesTransferred = 0;
+	      throw iioe;
+	    }
+	  // Some errors should not cause exceptions.
+	  if (errno != ENOTCONN && errno != ECONNRESET && errno != EBADF)
+	    throw new java::io::IOException (JvNewStringUTF (strerror (errno)));
+	}
+    }
+}
+
+// Write some bytes to the socket.
+void
+java::net::PlainSocketImpl::write(jbyteArray b, jint offset, jint len)
+{
+  if (! b)
+    throw new java::lang::NullPointerException;
+  if (offset < 0 || len < 0 || offset + len > JvGetArrayLength (b))
+    throw new java::lang::ArrayIndexOutOfBoundsException;
+
+  jbyte *bytes = elements (b) + offset;
+  int written = 0;
+  while (len > 0)
+    {
+      int r = ::write (fnum, bytes, len);
+      if (r == -1)
+        {
+	  if (java::lang::Thread::interrupted())
+	    {
+	      java::io::InterruptedIOException *iioe
+		= new java::io::InterruptedIOException
+		(JvNewStringLatin1 (strerror (errno)));
+	      iioe->bytesTransferred = written;
+	      throw iioe;
+	    }
+	  // Some errors should not cause exceptions.
+	  if (errno != ENOTCONN && errno != ECONNRESET && errno != EBADF)
+	    throw new java::io::IOException (JvNewStringUTF (strerror (errno)));
+	}
+      written += r;
+      len -= r;
+      bytes += r;
+    }
+}
+
+
+// Read a single byte from the socket.
+jint
+java::net::PlainSocketImpl::read(void)
+{
+  jbyte b;
+
+  // Do timeouts via select.
+  if (timeout > 0)
+  {
+    // Create the file descriptor set.
+    fd_set read_fds;
+    FD_ZERO (&read_fds);
+    FD_SET (fnum,&read_fds);
+    // Create the timeout struct based on our internal timeout value.
+    struct timeval timeout_value;
+    timeout_value.tv_sec = timeout / 1000;
+    timeout_value.tv_usec = (timeout % 1000) * 1000;
+    // Select on the fds.
+    int sel_retval = _Jv_select (fnum + 1, &read_fds, NULL, NULL, &timeout_value);
+    // If select returns 0 we've waited without getting data...
+    // that means we've timed out.
+    if (sel_retval == 0)
+      throw new java::io::InterruptedIOException
+	(JvNewStringUTF ("read timed out") );
+    // If select returns ok we know we either got signalled or read some data...
+    // either way we need to try to read.
+  }
+  int r = ::read (fnum, &b, 1);
+
+  if (r == 0)
+    return -1;
+  if (java::lang::Thread::interrupted())
+    {
+      java::io::InterruptedIOException *iioe =
+	new java::io::InterruptedIOException
+	(JvNewStringUTF("read interrupted"));
+      iioe->bytesTransferred = r == -1 ? 0 : r;
+      throw iioe;
+    }
+  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)));
+    }
+  return b & 0xFF;
+}
+
+// Read count bytes into the buffer, starting at offset.
+jint
+java::net::PlainSocketImpl::read(jbyteArray buffer, jint offset, jint count)
+{
+  if (! buffer)
+    throw new java::lang::NullPointerException;
+  jsize bsize = JvGetArrayLength (buffer);
+  if (offset < 0 || count < 0 || offset + count > bsize)
+    throw new java::lang::ArrayIndexOutOfBoundsException;
+  jbyte *bytes = elements (buffer) + offset;
+
+  // Do timeouts via select.
+  if (timeout > 0)
+  {
+    // Create the file descriptor set.
+    fd_set read_fds;
+    FD_ZERO (&read_fds);
+    FD_SET (fnum, &read_fds);
+    // Create the timeout struct based on our internal timeout value.
+    struct timeval timeout_value;
+    timeout_value.tv_sec = timeout / 1000;
+    timeout_value.tv_usec =(timeout % 1000) * 1000;
+    // Select on the fds.
+    int sel_retval = _Jv_select (fnum + 1, &read_fds, NULL, NULL, &timeout_value);
+    // We're only interested in the 0 return.
+    // error returns still require us to try to read 
+    // the socket to see what happened.
+    if (sel_retval == 0)
+      {
+	java::io::InterruptedIOException *iioe =
+	  new java::io::InterruptedIOException
+	  (JvNewStringUTF ("read interrupted"));
+	iioe->bytesTransferred = 0;
+	throw iioe;
+      }
+  }
+  // Read the socket.
+  int r = ::recv (fnum, bytes, count, 0);
+  if (r == 0)
+    return -1;
+  if (java::lang::Thread::interrupted())
+    {
+      java::io::InterruptedIOException *iioe =
+	new java::io::InterruptedIOException
+	(JvNewStringUTF ("read interrupted"));
+      iioe->bytesTransferred = r == -1 ? 0 : r;
+      throw iioe;
+    }
+  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)));
+    }
+  return r;
+}
+
+// How many bytes are available?
+jint
+java::net::PlainSocketImpl::available(void)
+{
+#if defined(FIONREAD) || defined(HAVE_SELECT)
+  long num = 0;
+  int r = 0;
+  bool num_set = false;
+
+#if defined(FIONREAD)
+  r = ::ioctl (fnum, FIONREAD, &num);
+  if (r == -1 && errno == ENOTTY)
+    {
+      // If the ioctl doesn't work, we don't care.
+      r = 0;
+      num = 0;
+    }
+  else
+    num_set = true;
+#elif defined(HAVE_SELECT)
+  if (fnum < 0)
+    {
+      errno = EBADF;
+      r = -1;
+    }
+#endif
+
+  if (r == -1)
+    {
+    posix_error:
+      throw new java::io::IOException(JvNewStringUTF(strerror(errno)));
+
+    }
+
+  // If we didn't get anything we can use select.
+
+#if defined(HAVE_SELECT)
+  if (! num_set)
+    {
+      fd_set rd;
+      FD_ZERO (&rd);
+      FD_SET (fnum, &rd);
+      struct timeval tv;
+      tv.tv_sec = 0;
+      tv.tv_usec = 0;
+      r = _Jv_select (fnum + 1, &rd, NULL, NULL, &tv);
+      if(r == -1)
+	goto posix_error;
+      num = r == 0 ? 0 : 1;
+    }
+#endif /* HAVE_SELECT */
+
+  return(jint) num;
+#else
+  throw new java::io::IOException (JvNewStringUTF ("unimplemented"));
+#endif
+ }
+
 
 void
 java::net::PlainSocketImpl::setOption (jint optID, java::lang::Object *value)