On Tue, 22 Nov 2022 17:30:56 GMT, Daniel Fuchs <[email protected]> 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
