Bug: SelectionKey.interestOps ignores interestOps on Windows

[Frank Ding]

Hi Alan,
   Thank you for your careful review.  I agree the scenario could happen.
   Would it be better if ski.setIndex(-1) in implDereg method and 
sk.getIndex() in putEventOps method are guarded by closeLock from JMM 
visibility point of  perspective?  The patch is available in the 02 
patch below.

   http://cr.openjdk.java.net/~dingxmin/6429204/webrev.02/

   Best regards,
   Frank

On 2/5/2013 11:33 PM, Alan Bateman wrote:
> On 04/02/2013 07:57, Frank Ding wrote:
>> Hi Alan,
>>   Thanks for your comments on the patch and the test case.
>>   I did more investigation on the  synchronization policy in Windows 
>> selector.
>>   Currently, closeLock is used in 3 methods of WindowsSelectorImpl, 
>> which are implClose, putEventOps, implRegister.  There are several 
>> vars guarded by closeLock in these 3 methods such as pollWrapper, 
>> channelArray, totalChannels, fdMap and some others.  Methods 
>> implClose, implRegister and implDereg are called by SelectorImpl 
>> which are all guarded by publicKeys.  The contention arises in this 
>> case between putEventOps and implDereg.  putEventOps is called by 
>> SelectionKey.interestOps where only closeLock is acquired and var 
>> pollWrapper is affected.  As a conclusion, I incline to keep the sync 
>> block as it is.
> I think there is still an issue, albeit remote.
>
> Consider an asynchronous interstOps at around the time that a key is 
> canceled and a thread is doing a select and processing the canceled 
> key set. It's possible that the thread that called interestOps has 
> already checked that the key is valid so we have implDereg and 
> putEventsOps running concurrently. In that scenario it is possible for 
> selection key's index to be set to -1 at around the time that the poll 
> wrapper is updated at that index.
>
> So I think the synchronized block need to be extended to 
> ski.setIndex(-1) and I think that putEventOps needs to be updated to 
> check the index, ie:
>
> int index = ski.getIndex();
> if (index == -1)
>     throw new CancelledKeyException();
>
> I don't think this scenario arises with your test but hopefully you 
> agree that this scenario is still possible.
>
> On the test - thanks for the cleanups and moving it, it looks much 
> better now.
>
> -Alan.
>