RFR: 8223783: sun/net/www/http/HttpClient/MultiThreadTest.java sometimes detect threads+1 connections [v4]

[Daniel Jeliński]

On Tue, 22 Nov 2022 17:30:56 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> Please find here a test fix for the sun/net/www/http/HttpClient/MultiThreadTest.
>> 
>> This test makes concurrent connections to a server using multiple threads, and due to keep-alive, expects that there should not be more connections than concurrent threads. However, the test has been seen occasionally and intermittently failing because it observed more connections than threads. The test has some built-in logic to exclude connections that do not come from the test itself, so external interference should not be the cause.
>> 
>> The suspicion is that occasionally some connection might remain idle for more than the keep-alive timeout, which then causes it to be closed, and allows the client to open a new connection.
>> 
>> This fix works this way: 
>> 
>> - first it adds some logic to figure out whether a Worker thread might have remained idle for more than the keep-alive timeout
>> - then it takes this information into account when matching the number of actual connections with the number of expected connection,
>> - additionally it reduces the keep-alive timeout to one second, and forces some sleep on the client side after half the requests have been sent to increase the probability that some connections will be idle long enough to trigger the idle-time detection logic (as a means to test the fix itself). 
>> 
>> Finally it improves logging by adding more diagnosis (including human readable time stamps), so that future failures, if any, will be easier to diagnose. Note that reducing the keep-alive time to 1 second makes the test run much faster even with the 1500ms sleep on the client side.
>> 
>> With this fix I no longer observe the test failing.
>
> Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Incorporated review feedback

Changes requested by djelinski (Committer).

test/jdk/sun/net/www/http/HttpClient/MultiThreadTest.java line 28:

> 26:  * @bug 4636628
> 27:  * @summary HttpURLConnection duplicates HTTP GET requests when used with multiple threads
> 28:  * @run main MultiThreadTest

do we really need to run this test 5 times?

test/jdk/sun/net/www/http/HttpClient/MultiThreadTest.java line 183:

> 181:         try {
> 182:             Object lock = MultiThreadTest.getLock();
> 183:             List<MultiThreadTest> tests = new ArrayList<>();

this list is not used

test/jdk/sun/net/www/http/HttpClient/MultiThreadTest.java line 253:

> 251: 
> 252:             if (validConnections != cnt) {
> 253:                 // these connections are not necessarily unexpected if SLEEP exceeds keep-alive.

Well actually these are unexpected connections; every connection from the tested client will equally count towards `cnt` and `validConnections`.

test/jdk/sun/net/www/http/HttpClient/MultiThreadTest.java line 263:

> 261:             debug("waiting for worker to shutdown at " + at() +"ms");
> 262:             for (Worker worker : svr.workers()) {
> 263:                 // we could call worker.done(); to force the worker

The `done()` method is unused, and as you pointed out here, we don't want to use it; can you remove it?

test/jdk/sun/net/www/http/HttpClient/MultiThreadTest.java line 390:

> 388:         volatile int timeoutExceeded;
> 389:         // Number of requests handled by this worker
> 390:         volatile int requestHandled;

not used

test/jdk/sun/net/www/http/HttpClient/MultiThreadTest.java line 423:

> 421:          * than the KEEP_ALIVE timeout. This will be true if the worker detected
> 422:          * that the idle timeout was exceeded between two consecutive request, or
> 423:          * if the time between the last reply

this needs cleanup

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

PR: https://git.openjdk.org/jdk/pull/11268