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]

[Jaikiran Pai]

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

>> The main thing for this test is that it's easy to maintain because someone looking at the test in a few years (maybe because of a test failure) will want to understand it quickly. I assume it doesn't need to be a public static field, it could be a private instance field. 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. I don't want to water Jai's time on too many iterations on this test, I think the main thing is that it is reliable and easy to maintain.
>
> 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). That was the reason why I chose to have this in the beforeAll/afterAll, to keep it relatively simple.

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

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