On Thu, 14 Nov 2024 12:39:39 GMT, Jaikiran Pai <j...@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. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Mark's suggestion - use try-with-resources for ServerSocket too Hello Mark, > is it the case that a try-with-resource, and the implicit final declaration > of s2, will maintain a strongly reachable reference ? > Rather than having to declare s2 as a final member variable of > Socket_getInputStream_read, as done with the reader socket s, or using a > ReachabilityFence to maintain an extant reference. The use (or non-use) of `final` doesn't play a role here. The use of try-with-resources for the `s2` `Socket` instance (the one which was being cleaned up by the `Cleaner`) is what enforces the strong reachability. The JLS (linked in the PR description) explains how the try-with-resources is implemented - specifically, the compiler introduces bytecode to have an explicit call to `s2.close()` at the end of that try-with-resources block (this can be checked by running `javap` against these updated classes). The presence of that `s2.close()` call is what enforces the reachability of `s2`. > I terms of "cleanliness" the listener ServerSocket is implicitly released I have now updated the PR to use a try-with-resources for the `ServerSocket` too. The tests continue to pass with these changes. ------------- PR Comment: https://git.openjdk.org/jdk/pull/22093#issuecomment-2476261110
