Re: Alpha Patch: Selector.select() Busted in 3.4

[Michael Koch <konqueror at gmx dot de>]

On Thu, Jan 29, 2004 at 12:24:13AM -0600, Mohan Embar wrote:
> Hi Michael (and whoever else is interested),
> 
> Although we got Selector.select() working at one point in time,
> it got busted before the 3.4 freeze because of this patch:
> 
> http://gcc.gnu.org/ml/java-patches/2004-q1/msg00052.html
> 
> I don't think the above patch itself is at fault, but the
> problem is Sun's confusing specs.
> 
> Here is an example of what's wrong. If you look at ServerSocket.accept(),
> Sun's JavaDoc says that an IllegalBlockingModeException should be thrown
> if the socket has an associated channel and it is non-blocking.
> 
> But here is an example of how one sets up a Selector.select() call
> (from my NetTest.java):
> 
>       ServerSocketChannel ssc = ServerSocketChannel.open();
>       arssc[nCurIndex++] = ssc;
>       ssc.configureBlocking(false);
>       ssc.socket().bind(new InetSocketAddress(nPort));
>       ssc.register(m_sel, SelectionKey.OP_ACCEPT);
>       // register interest in ACCEPT
> 
> This is consistent with this:
> 
> http://www.onjava.com/pub/a/onjava/2002/10/02/javanio.html?page=last
> 
> (see that section on Multiplexed I/O)
> 
> However, ServerSocket.implAccept() then bombs because its associated
> ServerSocketChannel has blocking = false.
> 
> I think the real specification to be followed is that of ServerSocketChannel.accept(),
> which overrides the specification of ServerSocket.accept() by saying that if blocking
> is false, the method should return immediately. But, as far as I can see, that means
> that in our implementation, ServerSocket.implAccept() needs to have some way of detecting
> that it is being called via ServerSocketChannel.accept() and ignoring the blocking flag.
> 
> The same applies for connect(), etc.
> 
> Here is an alpha patch I threw together to attempt to fix this. It's not
> breathtaking, but it makes NetTest pass again. While I was in here, I also fixed
> a couple of other minor things.
> 
> I did this against the 3.4 branch. I haven't done a ChangeLog because I wanted
> to see if you or anyone freaked out before I spent anymore time on this.
> Like I said, it's not amazing, but I think the only elegant way of solving the
> problem would be messing around with inner classes and stuff like that, and I
> was too lazy to be overly-elegant about this.

You are right with your observation. I found this some time ago but had
no real idea and notion to fix this. Its broken from the start and shows
e.g. when using freenet (thanx to Dalibor Topic for reporting ;-)).

I dont like the inIOOperation thingie but there is no better solution
exception "extending" SUNs API (AbstractSelectableChannel).

I'm okay with the patch and this surely has to go into trunk. I dont
think think this can go into branch but it should. Tom has to decide.

I have some formating nits and then this can go into trunk.

> -        socket.connect (remote, NIOConstants.DEFAULT_TIMEOUT);
> -        return true;
> +        inIOOperation = true;
> +        
> +        if (isBlocking())
> +          {
> +            // Do blocking connect.
> +            socket.connect (remote);

Leave the space before the parens. Our style is to not write it
(opposite to GNU coding style) (I know I told you the other way around
in the past). We should do it right in code we change and leave it in
other code and let jalopy do the hard work when we have it working.

>  import gnu.java.net.PlainSocketImpl;
> +import gnu.java.nio.ServerSocketChannelImpl;
> +
>  import java.io.IOException;
>  import java.nio.channels.IllegalBlockingModeException;

Please drop the empty line here.

>      synchronized (blockingLock())
>        {
> -        implConfigureBlocking(blocking);
> -        this.blocking = blocking;
> +        if (this.blocking != blocking)
> +          {
> +            implConfigureBlocking(blocking);
> +            this.blocking = blocking;
> +          }
>        }

Thanks for catching this too.


Michael