On Thu, 21 Nov 2024 10:47:54 GMT, Volkan Yazıcı <d...@openjdk.org> wrote:
>> This PR, addressing 8343791, changes `Socket::connect()` methods to close
>> the `Socket` in the event that the connection cannot be established, the
>> timeout expires before the connection is established, or the socket address
>> is unresolved.
>>
>> `tier3` tests pass against the 9f00f61d3b7fa42a5e23a04f80bb4bb1a2076ef2.
>
> 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 ?
test/jdk/java/net/Socket/CloseOnFailureTest.java line 210:
> 208: private static final String ERROR_MESSAGE = "intentional test
> failure";
> 209:
> 210: private static final class ForBindFailure {
ForConnectionFailure ForBindfFailure are factories abstraction
Their method are factory methods (the world is addicted to of methods)
fabricating a TestCase
ofIOException
ofIllegalArgumentException
ofIOException()
A simple change to ConnectionFailureFactory and BindFailureFactory
With methods like ioExceptionTestCase illegalArgumentException
Lead to readable code like
TestCase.ConnectionFailureFactory.ioExceptionTestCase()
TestCase.BindFaiureFactory.illegalExceptionTestCase()
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1852960421
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1852961418
