Re: [PATCH] [POSIX]: natSelectorImplPosix.cc Fixes

[Michael Koch <konqueror at gmx dot de>]

On Thu, Dec 11, 2003 at 02:06:13AM -0600, Mohan Embar wrote:
> Hi Michael,
> 
> >> P.S. I was trying to get natSelectorImplWin32 to work and wrote a cool
> >> test (no, not a Mauve one yet - I have my fingers in my ears already)
> >> which unconvered issues with selectors, etc. Do you want:
> >> 
> >> - me to investigate this?
> >> - me to give you the test and you investigate this?
> >> - both?
> >> 
> >> Let me know.
> >
> >I take door B (send me the test please). Its surely possible that there
> >are some bugs in the java part of NIO. I can fix them first and then
> >we/you can implement/submit/commit the native methods for Win32.
> 
> I know you said you would look into this, but my fingers started
> itching, and I couldn't help myself....

Damn, too much work for not enought time ...

> With this patch, the attached channels/selector testcase diffs almost
> identically between Sun's JRE on Linux and gcj. The only discrepancy
> is when querying the interrupt flag of the thread and I believe that gcj's
> behavior is more correct.
> 
> Let me know what you think and if/how you want to proceed.

Cool, that you take the step forward.

I haven't tested the patch yet but I have some remarks about it:

> 	* gnu/java/nio/SocketChannelImpl.java
> 	(write): Removed diagnostic trace.
> 	* gnu/java/nio/natSelectorImplPosix.cc: Added
> 	includes for java.lang.Thread and java.io.InterruptedIOException.
> 	(helper_put_filedescriptors): Don't put invalid file descriptors
> 	in select set.
> 	(helper_get_filedescriptors): Clear invalid file descriptors
> 	from select set.
> 	(helper_reset): New method for clearing our file descriptor
> 	array.
> 	(implSelect): Correctly calculate timeout if specified and
> 	legal.
> 	Intercept and deal with any java.io.InterruptedIOException
> 	thrown by _Jv_select().
> 
> Index: gnu/java/nio/SocketChannelImpl.java
> ===================================================================
> RCS file: /cvs/gcc/gcc/libjava/gnu/java/nio/SocketChannelImpl.java,v
> retrieving revision 1.13
> diff -u -2 -r1.13 SocketChannelImpl.java
> --- gnu/java/nio/SocketChannelImpl.java	2 Dec 2003 15:03:21 -0000	1.13
> +++ gnu/java/nio/SocketChannelImpl.java	11 Dec 2003 07:50:17 -0000
> @@ -302,6 +302,4 @@
>        }
>  
> -    System.out.println ("INTERNAL: writing to socket outputstream");
> -    

Obviously correct.

>      OutputStream output = socket.getOutputStream();
>      output.write (data, offset, len);
> Index: gnu/java/nio/natSelectorImplPosix.cc
> ===================================================================
> RCS file: /cvs/gcc/gcc/libjava/gnu/java/nio/natSelectorImplPosix.cc,v
> retrieving revision 1.3
> diff -u -2 -r1.3 natSelectorImplPosix.cc
> --- gnu/java/nio/natSelectorImplPosix.cc	4 Dec 2003 00:31:27 -0000	1.3
> +++ gnu/java/nio/natSelectorImplPosix.cc	11 Dec 2003 07:50:18 -0000
> @@ -17,4 +17,6 @@
>  #include <gnu/java/nio/SelectorImpl.h>
>  #include <java/io/IOException.h>
> +#include <java/io/InterruptedIOException.h>
> +#include <java/lang/Thread.h>

Please use alphabetical order.

>  static void
> @@ -25,8 +27,12 @@
>    for (int index = 0; index < JvGetArrayLength (fdArray); index++)
>      {
> -      FD_SET (tmpFDArray [index], &fds);
> -
> -      if (tmpFDArray [index] > max_fd)
> -        max_fd = tmpFDArray [index];
> +      int fd = tmpFDArray [index];
> +      if (fd > 0)
> +        {
> +          FD_SET (tmpFDArray [index], &fds);
> +
> +          if (tmpFDArray [index] > max_fd)
> +            max_fd = tmpFDArray [index];
> +        }
>      }
>  }
> @@ -38,6 +44,18 @@
>    
>    for (int index = 0; index < JvGetArrayLength (fdArray); index++)
> -    if (!FD_ISSET (tmpFDArray [index], &fds))
> -      tmpFDArray [index] = 0;
> +    {
> +      int fd = tmpFDArray [index];
> +      if (fd < 0 || !FD_ISSET (fd, &fds))
> +        tmpFDArray [index] = 0;
> +    }
> +}
> +
> +static void
> +helper_reset (jintArray& fdArray)
> +{
> +  jint* tmpFDArray = elements (fdArray);
> +  
> +  for (int index = 0; index < JvGetArrayLength (fdArray); index++)
> +    tmpFDArray [index] = 0;
>  }
>  
> @@ -54,13 +72,13 @@
>    struct timeval *time_data = NULL;
>  
> -  real_time_data.tv_sec = 0;
> -  real_time_data.tv_usec = timeout;
> -
> -  // If not legal timeout value is given, use NULL.
> +  // If a legal timeout value isn't given, use NULL.
>    // This means an infinite timeout. The specification
>    // also says that a zero timeout should be treated
> -  // as infinite.
> +  // as infinite. Otherwise (if the timeout value is legal),
> +  // fill our timeval struct and use it for the select.
>    if (timeout > 0)
>      {
> +      real_time_data.tv_sec = timeout / 1000;
> +      real_time_data.tv_usec = (timeout % 1000) * 1000;

Damn, this is very obvious to me and I scratched my head already on
this.

>        time_data = &real_time_data;
>      }
> @@ -77,5 +95,21 @@
>  
>    // Actually do the select
> -  result = _Jv_select (max_fd + 1, &read_fds, &write_fds, &except_fds, time_data);
> +  try
> +    {
> +      result = _Jv_select (max_fd + 1, &read_fds, &write_fds,
> +                           &except_fds, time_data);
> +    }
> +  catch (::java::io::InterruptedIOException *e)

Can't we do this in the Java code ? (in SelectorImpl.java)

> +    {
> +      // The behavior of JRE 1.4.1 is that no exception is thrown
> +      // when the thread is interrupted, but the thread's interrupt
> +      // status is set. Clear all of our select sets and return 0,
> +      // indicating that nothing was selected.
> +      ::java::lang::Thread::currentThread ()->interrupt ();
> +       helper_reset (read);
> +       helper_reset (write);
> +       helper_reset (except);

Do we really need this ? The helper arrays are supposed to be recreated
before each selectImpl() call.

> +       return 0;
> +    }
>  
>    if (result < 0)
> 


Michael