ok2c commented on code in PR #709: URL: https://github.com/apache/httpcomponents-client/pull/709#discussion_r2353121202
########## httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/TestConnectionClosureRace.java: ########## @@ -0,0 +1,448 @@ +/* + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * + * This software consists of voluntary contributions made by many + * individuals on behalf of the Apache Software Foundation. For more + * information on the Apache Software Foundation, please see + * <http://www.apache.org/>. + * + */ + +package org.apache.hc.client5.testing.async; + +import org.apache.hc.client5.http.async.methods.SimpleHttpRequest; +import org.apache.hc.client5.http.async.methods.SimpleHttpResponse; +import org.apache.hc.client5.http.async.methods.SimpleRequestBuilder; +import org.apache.hc.client5.http.config.ConnectionConfig; +import org.apache.hc.client5.http.config.TlsConfig; +import org.apache.hc.client5.http.impl.async.CloseableHttpAsyncClient; +import org.apache.hc.client5.http.impl.async.HttpAsyncClients; +import org.apache.hc.client5.http.impl.nio.PoolingAsyncClientConnectionManager; +import org.apache.hc.client5.http.impl.nio.PoolingAsyncClientConnectionManagerBuilder; +import org.apache.hc.client5.http.ssl.DefaultClientTlsStrategy; +import org.apache.hc.client5.testing.SSLTestContexts; +import org.apache.hc.core5.http.ConnectionClosedException; +import org.apache.hc.core5.http.HttpHost; +import org.apache.hc.core5.http.RequestNotExecutedException; +import org.apache.hc.core5.http.message.BasicHeader; +import org.apache.hc.core5.http.ssl.TLS; +import org.apache.hc.core5.http2.H2Error; +import org.apache.hc.core5.http2.H2PseudoResponseHeaders; +import org.apache.hc.core5.http2.HttpVersionPolicy; +import org.apache.hc.core5.http2.config.H2Param; +import org.apache.hc.core5.http2.config.H2Setting; +import org.apache.hc.core5.http2.frame.DefaultFrameFactory; +import org.apache.hc.core5.http2.frame.FrameType; +import org.apache.hc.core5.http2.hpack.HPackEncoder; +import org.apache.hc.core5.http2.impl.io.FrameInputBuffer; +import org.apache.hc.core5.http2.impl.io.FrameOutputBuffer; +import org.apache.hc.core5.reactor.IOReactorConfig; +import org.apache.hc.core5.util.ByteArrayBuffer; +import org.apache.hc.core5.util.TimeValue; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.MethodOrderer.OrderAnnotation; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Order; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestMethodOrder; +import org.junit.jupiter.api.Timeout; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import javax.net.ServerSocketFactory; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLServerSocketFactory; +import javax.net.ssl.SSLSocket; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.net.ServerSocket; +import java.net.Socket; +import java.net.SocketException; +import java.nio.ByteBuffer; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Future; +import java.util.concurrent.atomic.AtomicInteger; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.apache.hc.core5.http2.HttpVersionPolicy.FORCE_HTTP_1; +import static org.apache.hc.core5.http2.HttpVersionPolicy.FORCE_HTTP_2; +import static org.apache.hc.core5.util.TimeValue.MAX_VALUE; +import static org.apache.hc.core5.util.TimeValue.NEG_ONE_MILLISECOND; +import static org.apache.hc.core5.util.TimeValue.ZERO_MILLISECONDS; +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * This test exercises a race condition between client connection reuse and server-initiated connection closure. The + * test matrix consists of two protocols (HTTP/1.1 and HTTP/2), two connection layers (TCP and TLS 1.2), and two, + * settings for {@link ConnectionConfig#getValidateAfterInactivity}: -1 ms (never validate) and 0 ms (always validate). + * <p> + * The tests work by sending simple ping requests to a test server over a single connection. Requests are batched in + * various ways and sent at various rates. The test server closes the connection immediately upon sending a response. + * HTTP/1.1 connections are closed silently (without any {@code Connection: close} header), in order to simulate + * connection closure caused by idleness or by the network itself. HTTP/2 connections are closed with a {@code GOAWAY} + * frame. Logically, a TCP {@code FIN} on HTTP/1.1 and a {@code GOAWAY} frame on HTTP/2 both indicate that no additional + * requests should be sent on the connection, and this test case investigates under what circumstances we can handle + * that signal before leasing the connection to a new request. + * <p> + * The only thing the test actually asserts is that the requests don't time out; the client should never hang due to + * connection closure. Any additional assertions (such as minimum success rates) would be difficult to make reliably, + * since server-initiated connection closure is inherently prone to race conditions, even when testing with a single + * process talking to itself over localhost. However, each test case prints statistics showing the request success rate + * before and after enabling connection validation. This shows the effectiveness of our inactive connection validation + * strategy in mitigating races within the client. + */ +@TestMethodOrder(OrderAnnotation.class) +abstract class AbstractTestConnectionClosureRace { + final AtomicInteger connectionsEstablished = new AtomicInteger(0); + final String scheme; + final HttpVersionPolicy httpVersionPolicy; + + volatile ServerSocket serverSocket; + volatile int port; + volatile Thread serverThread; + + AbstractTestConnectionClosureRace(final String scheme, final HttpVersionPolicy httpVersionPolicy) { + this.scheme = scheme; + this.httpVersionPolicy = httpVersionPolicy; + } + + @BeforeAll + static void newline() { + System.out.println(); + } + + @BeforeEach + void setup() throws Exception { + if ("http".equals(scheme)) { + serverSocket = ServerSocketFactory.getDefault().createServerSocket(0); + } else { + final SSLContext serverSSLContext = SSLTestContexts.createServerSSLContext(); + final SSLServerSocketFactory sslServerSocketFactory = serverSSLContext.getServerSocketFactory(); + serverSocket = sslServerSocketFactory.createServerSocket(0); + } + port = serverSocket.getLocalPort(); + + serverThread = new Thread(this::runServer); + serverThread.setDaemon(true); + serverThread.start(); + } + + @AfterEach + void tearDown() throws Exception { + if (serverSocket != null) { + serverSocket.close(); + } + if (serverThread != null) { + serverThread.interrupt(); + serverThread.join(1000); + } + } + + @Test + @Timeout(5) + @Order(1) + void smokeTest() throws Exception { + try (final CloseableHttpAsyncClient client = asyncClient(false)) { + final SimpleHttpResponse simpleHttpResponse = sendPing(client).get(); + assertEquals(200, simpleHttpResponse.getCode()); + } + } + + @ParameterizedTest(name = "Validation: {0}") + @ValueSource(booleans = { false, true }) + @Timeout(5) + @Order(2) + void testSlowSequentialRequests(final boolean validateConnections) throws Exception { + try (final CloseableHttpAsyncClient client = asyncClient(validateConnections)) { + final List<Future<SimpleHttpResponse>> futures = new ArrayList<>(); + for (int i = 0; i < 10; i++) { + futures.add(sendPing(client)); + Thread.sleep(25); + } + + checkResults(getValidationPrefix(validateConnections) + "Sequential requests (slow)", futures); + } + } + + @ParameterizedTest(name = "Validation: {0}") + @ValueSource(booleans = { false, true }) + @Timeout(10) + @Order(3) + void testRapidSequentialRequests(final boolean validateConnections) throws Exception { + try (final CloseableHttpAsyncClient client = asyncClient(validateConnections)) { + final List<Future<SimpleHttpResponse>> futures = new ArrayList<>(); + for (int i = 0; i < 2500; i++) { + final Future<SimpleHttpResponse> f = sendPing(client); Review Comment: > testRapidSequentialRequests could Thread.sleep(25) instead, but blocking on the future is faster. @rschmitt I still do not understand why all tests could not behave consistently, and why a faster future is better than Thread.sleep(25) but so be it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org For additional commands, e-mail: dev-h...@hc.apache.org
