RFR: 8343877: Test AsyncClose.java intermittent fails - Socket.getInputStream().read() wasn't preempted

[Alan Bateman]

On Thu, 14 Nov 2024 04:56:12 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

> Can I please get a review of this test-only fix which addresses the intermittent failure in AsyncClose test as noted in https://bugs.openjdk.org/browse/JDK-8343877?
> 
> The `FileDescriptor` of the `Socket` instance that gets returned from a call to `ServerSocket.accept()` gets enrolled with the `java.lang.ref.Cleaner` so that the underlying file descriptor of the newly accepted socket gets closed.
> 
> The `Socket_getInputStream_read` and even the `Socket_getOutputStream_write` tests don't hold on to the `Socket` instance returned by the `accept()` call until the concurrent `Thread` that reads (or writes) on the client side of the socket. This is an oversight in the test and this can lead to a case where the GC upon noticing that the returned `Socket` instance isn't referenced may garbage collect that instance and thus the `Cleaner` may end up closing the underlying file descriptor. When that happens, the expectations of the test don't hold and thus the test fails as noted in the linked issue.
> 
> The commit in this PR updates these 2 tests to use a try-with-resources for the returned `Socket` instance and that way enforce reachability of the `Socket` instance until the end of the try block. This should prevent the GC from garbage collecting the instance and at the same time `close()` the returned `Socket` in a deterministic manner.
> 
> No explicit `java.lang.ref.Reference.reachabilityFence()` should be required here given what the JLS specifies about try-with-resources and reachability in section 14.20.3 https://docs.oracle.com/javase/specs/jls/se23/html/jls-14.html#jls-14.20.3.
> 
> These tests continue to pass after this change with a test repeat of 50. tier2 too passes without any failures.

Yes, good sleuthing. I think this is a JDK 1.4 era test so this test bug may have existed since then. In passing I wonder if we can replace the sleeps with code that we use in other tests to poll the thread's stack until it is observed in the expected method. Not for this PR of course.

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

Marked as reviewed by alanb (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22093#pullrequestreview-2435502047