On Thu, 21 Nov 2024 21:54:46 GMT, Mark Sheppard <mshep...@openjdk.org> wrote:
>> Volkan Yazıcı has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Handle `SocketTimeoutException` in `NioSocketImpl::connect()` > > test/jdk/java/net/Socket/CloseOnFailureTest.java line 139: > >> 137: // They just need to be _valid enough_ to reach to the >> point where both `SocketImpl#bind()` and `SocketImpl#connect()` are invoked. >> 138: // Failure will be triggered by the injected >> `SocketImpl`. >> 139: new Socket(address, 0xDEAD, address, 0xBEEF); > > Using new Socket(address, 0xDEAD, address, 0xBEEF); using hex as port > number, a bit too clever! > > I think a constant would be preferable for us simple folk. > > private static final int DEADSERVERPORT = 0xDEAD; // or 57005 > > I think it is preferable for the local port to use 0 i.e. an OS ephemeral > allocation, as it is moot in this test context > > One observation I’d make on this is that, when scanning the code and not > carrying the details of the MockSocketImpl with you, one is inclined to think > that this will intermittently lead to BindException and ConnectException as > both 0xDEAD and 0xBEEF are in the ephemeral port range > > Thus, a general question on the scope of testing. When considering the use of > mock object in the call flows. Are these tests sufficient for the testing > that the spec changes are upheld by the full default implementation ? Applied these suggestions in e37ee840b9681aa017f8653df68c69232a6b6c83. > One observation I’d make on this is that, when scanning the code and not > carrying the details of the MockSocketImpl with you, one is inclined to think > that this will intermittently lead to BindException and ConnectException as > both 0xDEAD and 0xBEEF are in the ephemeral port range Agreed. That's why I documented the choice of address and port just above the line calling the ctor. The new code you suggested using variables instead of hardcoded constants should further help with this. > Thus, a general question on the scope of testing. When considering the use of > mock object in the call flows. Are these tests sufficient for the testing > that the spec changes are upheld by the full default implementation? That is a very good question! Judging from [the CSR](https://bugs.openjdk.org/browse/JDK-8344527), the specification changes concern: 1. `SocketImpl::connect()` failures (the connection cannot be established or the timeout expires before the connection is established) 2. `Socket::connect(SocketAddress)` is passed an unresolved address (1) is verified using `connectShouldCloseOnFailures()`, and (2) is verified using `connectShouldCloseOnUnresolvedAddress()`. If, at a later time, `Socket::connect()` gets modified to **not** use the `SocketImpl`, and invalidate the assumptions held in this test, associated test methods will fail, and force the author to take the CSR requirements into account, AFAICT. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1853644755
