RFR[11] of JDK-8208280,java/nio/channels/Selector/RegisterDuringSelect.java fails with "key not removed from key set"

[Hamlin Li]

Thank you for detailed reviewing, I have modified as you suggested, 
please review it again: http://cr.openjdk.java.net/~mli/8208280/webrev.03/

Thank you

-Hamlin


On 2018/8/29 6:05 PM, Alan Bateman wrote:
> On 10/08/2018 06:14, Hamlin Li wrote:
>>
>>
>> On 2018/8/9 6:51 PM, Alan Bateman wrote:
>>>
>>> No objection to dropping the use of phaser from this test but I'm 
>>> concerned that this version isn't providing any guarantee that it's 
>>> testing that these methods can be called while a selection operation 
>>> is in progress. It's similar to the discussion we had here about the 
>>> SelectAndClose test in (JDK-8203765) where we converged on spinning 
>>> until the the selectedKey set was locked as an indication that the 
>>> selection operation is in progress. I think we can do something 
>>> similar here. I'm happy to take this one if you'd like.
>> Sure, it's updated, please help to review it: 
>> http://cr.openjdk.java.net/~mli/8208280/webrev.02/
> This looks much better.
>
> It would be good to put javadoc style method description on 
> mightHoldLock and spinUntilLock as they will be used by several tests. 
> Also I assume you have a typo in the method name and it should be 
> "spinUntilLocked".
>
> The update to the SelectAndClose test looks okay.
>
> The re-implementation of the RegisterDuringSelector test is okay but I 
> think needs a bit of clean-up to make it easier to maintain. One 
> suggestion is to add this function:
>
>     interface TestOperation {
>         void accept(Thread t, Selector sel) throws IOException;
>     }
>
> Then rename Select to Test have it define a test(TestOperation op) 
> method. That will allow to use a lambda expression and needing to 
> override readTest in each of the tests. Also having the Thread and 
> Selector provided as parameters makes it easier to see where they are 
> coming from.
>
> Another thing is that sel and the other fields should be final.
>
> -Alan
>
> -Alan
>
>
>
>