Request for review / feedback: SocketChannel.poll()

[David M. Lloyd]

On 10/02/2013 04:03 PM, David M. Lloyd wrote:
> 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.

I mean "consistency is worthless...".

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


-- 
- DML