On Mon, 16 Jun 2025 07:46:33 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Jaikiran Pai has updated the pull request incrementally with four additional
>> commits since the last revision:
>>
>> - include a test for AsynchronousServerSocketChannel
>> - System.err instead of System.out
>> - trim down code comment
>> - > 200 instead of >= 200
>
> test/jdk/java/net/ServerSocket/LargeBacklogTest.java line 43:
>
>> 41: * @run junit LargeBacklogTest
>> 42: */
>> 43: class LargeBacklogTest {
>
> How reliable is this test? If it is reliable then we can add tests for
> SocketChannelChannel::socket and AsynchronousServerSocket too.
I have run this test over a 1000 times in our CI and it hasn't failed even
once. The test methods themselves complete in a few milli seconds:
SUCCESSFUL LargeBacklogTest::testServerSocket 'testServerSocket()' [180ms]
SUCCESSFUL LargeBacklogTest::testServerSocketChannel
'testServerSocketChannel()' [92ms]
SUCCESSFUL LargeBacklogTest::testAsynchronousServerSocketChannel
'testAsynchronousServerSocketChannel()' [88ms]
so the speed is good too. Plus, the test has been written in a manner where it
allows a some leeway when asserting for the backlogged connection count for the
cases where some unexpected process on the host might attempt a connection to
this server. Given this, I think the test is reliable.
> If it is reliable then we can add tests for SocketChannelChannel::socket and
> AsynchronousServerSocket too.
I've update the PR to add one for `AsynchronousServerSocket`. It already had
test for `ServerSocket` and `ServerSocketChannel`. As for
`SocketChannelChannel::socket`, that appears to be a typo. Is there any other
channel type you wanted to be tested too?
> test/jdk/java/net/ServerSocket/LargeBacklogTest.java line 72:
>
>> 70: private static void testBackloggedConnects(final int backlog, final
>> int serverPort) {
>> 71: int numSuccessfulConnects = 0;
>> 72: System.out.println("attempting " + backlog + " connections to
>> port " + serverPort);
>
> I don't know if this is a left over trace message or not. If intended then
> you might have to change it to use System.err so that it inlines with the
> test (at least I think JUnit uses System.err, TestNG uses System.out).
I hadn't paid attention to this detail, but yes you are right the junit logs -
`STARTED ...`, `SUCCESSFUL ....`, `FAILED ...` appear on System.err. So yes,
having these rest of the logs appear there would be a good thing. I have
updated the PR to replace System.out with System.err.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25819#discussion_r2149522443
PR Review Comment: https://git.openjdk.org/jdk/pull/25819#discussion_r2149526743
