RFR: 8334719: (se) Deferred close of SelectableChannel may result in a Selector doing the final close before concurrent I/O on channel has completed [v5]

[Alan Bateman]

On Wed, 26 Jun 2024 11:48:14 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Hello Daniel, I don't think it's needed. We set this to `true` (only) once in a `@BeforeAll` which gets called by one single thread. The subsequent multiple threads will only get created and launched as part of the test methods after the `@BeforeAll` has finished, so I believe they would all correctly see enableDelay as true.
>> 
>> After that, we only update that `enableDelay` value after all tests are done, so that too should be fine.
>
>> I assume it doesn't need to be a public static field, it could be a private instance field.
> 
> It should have been a private field. It's an oversight that I left it as public in a recent update. I'll fix that.
> 
>> I think that would remove the need for the beforeAll/afterAll as it would allow the test to just set the delay for the ISA used by the test.
> 
> The important detail here is that to consistently reproduce this issue both in send and receive methods of the DatagramChannelImpl, the delay has to be introduced not just in the ISA constructed by the test methods, but also in the ISA constructed by the DatagramChannelImpl implementation's decoding of native addresses to ISA. That decoding of ISA happens deep within the receive operation after it has acquired the relevant lock and that's the place where we want to slow things down so that a concurrent thread can then enter and finish close the channel which was in deferred close state.
> 
> If we wanted to reproduce the issue only for send, then constructing the delayed ISA in the test method should have been enough (because send would just call the ISA.getPort() on the ISA passed by the test method). But I also wanted to be sure that there was a way to reproduce (and thus verify the fix) for receive too (which internally calls the ISA constructor at least once upon receiving a datagram and converting the native source address to the ISA). That was the reason why I chose to have this in the beforeAll/afterAll, to keep it relatively simple.

Okay, I think we can live with that.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/19879#discussion_r1654848691