Request for review / feedback: SocketChannel.poll()

[David M. Lloyd]

On 10/02/2013 02:14 PM, Rob McKenna wrote:
> Hi folks,
>
> After adding Net.poll() in 7184932, Alan suggested I implement a poll
> method in SocketChannel to allow for the temporary use of non-blocking
> SocketChannels in a blocking fashion. So this is my first public effort
> and is likely quite rough around the edges.
>
> Apologies for the rushed approach, but I'd really like to gather
> feedback as soon as possible as we're running quite late for 8. (its
> been a busy couple of months unfortunately)
>
> http://cr.openjdk.java.net/~robm/sc.poll/webrev.01/
>
> One thing to note is that in the implementation of poll we have 3 calls
> to Net.poll() within 3 conditionals. The reason for this is to allow for
> simultaneous calls to sc.poll(OP_READ) and sc.poll(OP_WRITE).
>
> Another part in particular that I'd like to highligh is the awkward
> timeout. The spec does say that this method will block if (e.g.) a read
> operation is performed in another thread and we poll for OP_READ in our
> thread. That is to say, the timeout only comes into play once we have
> the appropriate locks. I've also followed the poll system calls
> semantics for the timeout parameter. (<0: block indefinitely, 0: return
> immediately, >0: block for specified interval)
>
> The test is still a work in progess, but any feedback at all would be
> gratefully received.

I am not an official reviewer.

There are a couple issues with the JavaDoc:

1. "...{@code OP_CONNECT} then the poll may not proceed concurrently 
with either a read or write operation" - no trailing period
2. @param/@return/@throws should probably be consistent with 
capitalization (@throws phrases start with caps but others do not; 
probably no phrases should, though maybe this is some weird convention 
you guys have)

A couple semantic issues:

1. Thread interruption should stop the poll, but absolutely should not 
*not* close the channel - that was a bad idea at its inception and it is 
still a bad idea now, and people who do not want this behavior (== sane 
people) will still be stuck with using temporary selectors.  It is up to 
the application to decide what thread interruption means. 
InterruptedIOException is more appropriate, and the user can elect to 
close the channel if they feel it is appropriate to do so. 
Inconsistency is worthless if the existing behavior does not make sense.

2. Not a fan of the triple-behavior-overload of the timeout parameter.

-- 
- DML