From c14b92b8e0040a10f4c7b5554974184b52b3375b Mon Sep 17 00:00:00 2001 From: Bryce Anderson Date: Mon, 2 Dec 2024 15:45:26 -0700 Subject: [PATCH 01/13] Repeat a flaky test --- .../http/TrafficResilienceHttpServiceFilterTest.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java index e37315fc5b..b6818dbbac 100644 --- a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java +++ b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java @@ -44,6 +44,7 @@ import io.servicetalk.http.netty.HttpServers; import io.servicetalk.transport.api.ServerContext; +import org.junit.jupiter.api.RepeatedTest; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; @@ -200,6 +201,12 @@ public TicketObserver onAllowedThrough(StreamingHttpRequest request, CapacityLim assertThat(rejectedCount.get(), equalTo(1)); } + @Test + @RepeatedTest(100) + void repro() throws Exception { + testStopAcceptingConnections(false, "h1"); + } + @ParameterizedTest(name = "{displayName} [{index}] dryRun={0},protocol={1}") @CsvSource({"true, h1", "true, h2", "false, h1", "false, h2"}) void testStopAcceptingConnections(final boolean dryRun, final String protocol) throws Exception { From dddf0a91bbb79f2b553443d7c48f4c95cc688469 Mon Sep 17 00:00:00 2001 From: Bryce Anderson Date: Mon, 2 Dec 2024 16:38:04 -0700 Subject: [PATCH 02/13] See if we can side-step the flakiness --- .../http/TrafficResilienceHttpServiceFilterTest.java | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java index b6818dbbac..fdf9714dd4 100644 --- a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java +++ b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java @@ -260,8 +260,15 @@ void testStopAcceptingConnections(final boolean dryRun, final String protocol) t .toFuture().get().asConnection(), instanceOf(HttpConnection.class)); // This connection shall full-fil the BACKLOG=1 setting - assertThat(client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")) - .toFuture().get().asConnection(), instanceOf(HttpConnection.class)); + try { + assertThat(client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")) + .toFuture().get().asConnection(), instanceOf(HttpConnection.class)); // This is the failing line. https://github.com/apple/servicetalk/actions/runs/12129341561/job/33817567364?pr=3125 + } catch (ExecutionException e) { + if (dryRun) { + throw e; + } + assertThat(e.getCause(), instanceOf(ConnectTimeoutException.class)); + } // Any attempt to create a connection now, should time out if we're not in dry mode. if (dryRun) { From 97d3570c4d91250d629302e114f6ed015e81cb09 Mon Sep 17 00:00:00 2001 From: Bryce Anderson Date: Mon, 2 Dec 2024 16:55:41 -0700 Subject: [PATCH 03/13] Fix style check --- .../http/TrafficResilienceHttpServiceFilterTest.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java index fdf9714dd4..1cdf9caf47 100644 --- a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java +++ b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java @@ -262,7 +262,9 @@ void testStopAcceptingConnections(final boolean dryRun, final String protocol) t // This connection shall full-fil the BACKLOG=1 setting try { assertThat(client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")) - .toFuture().get().asConnection(), instanceOf(HttpConnection.class)); // This is the failing line. https://github.com/apple/servicetalk/actions/runs/12129341561/job/33817567364?pr=3125 + // This is the failing line. + // https://github.com/apple/servicetalk/actions/runs/12129341561/job/33817567364?pr=3125 + .toFuture().get().asConnection(), instanceOf(HttpConnection.class)); } catch (ExecutionException e) { if (dryRun) { throw e; From 12b0a48c1584cf836801e62991812b6b6b880e91 Mon Sep 17 00:00:00 2001 From: Bryce Anderson Date: Mon, 2 Dec 2024 17:47:24 -0700 Subject: [PATCH 04/13] See where the test is now hanging --- .../http/TrafficResilienceHttpServiceFilterTest.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java index 1cdf9caf47..c4b10e6318 100644 --- a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java +++ b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java @@ -51,6 +51,7 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; import static io.netty.util.internal.PlatformDependent.normalizedOs; @@ -261,10 +262,11 @@ void testStopAcceptingConnections(final boolean dryRun, final String protocol) t // This connection shall full-fil the BACKLOG=1 setting try { + System.out.println("Attempting to get another connection"); assertThat(client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")) // This is the failing line. // https://github.com/apple/servicetalk/actions/runs/12129341561/job/33817567364?pr=3125 - .toFuture().get().asConnection(), instanceOf(HttpConnection.class)); + .toFuture().get(CI ? 4 : 2, SECONDS).asConnection(), instanceOf(HttpConnection.class)); } catch (ExecutionException e) { if (dryRun) { throw e; @@ -272,6 +274,7 @@ void testStopAcceptingConnections(final boolean dryRun, final String protocol) t assertThat(e.getCause(), instanceOf(ConnectTimeoutException.class)); } + System.out.println("Now we need to fail the connection."); // Any attempt to create a connection now, should time out if we're not in dry mode. if (dryRun) { client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")).toFuture().get() From f628240f03777aa1d692ad55ebfc4b6167ffc618 Mon Sep 17 00:00:00 2001 From: Bryce Anderson Date: Mon, 2 Dec 2024 20:25:24 -0700 Subject: [PATCH 05/13] See if we can extend the timeout --- .../java/io/servicetalk/loadbalancer/DefaultLoadBalancer.java | 1 + .../http/TrafficResilienceHttpServiceFilterTest.java | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancer.java b/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancer.java index 14d6881677..f06273993e 100644 --- a/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancer.java +++ b/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancer.java @@ -559,6 +559,7 @@ public Single newConnection(@Nullable final ContextMap context) { private Single selectConnection0(final Predicate selector, @Nullable final ContextMap context, final boolean forceNewConnectionAndReserve) { final HostSelector currentHostSelector = hostSelector; + LOGGER.info("Selecting connection."); Single result = currentHostSelector.selectConnection(selector, context, forceNewConnectionAndReserve); return result.beforeOnError(exn -> { if (exn instanceof NoActiveHostException) { diff --git a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java index c4b10e6318..4694b9336e 100644 --- a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java +++ b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java @@ -266,7 +266,8 @@ void testStopAcceptingConnections(final boolean dryRun, final String protocol) t assertThat(client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")) // This is the failing line. // https://github.com/apple/servicetalk/actions/runs/12129341561/job/33817567364?pr=3125 - .toFuture().get(CI ? 4 : 2, SECONDS).asConnection(), instanceOf(HttpConnection.class)); + // This now fails to resolve. + .toFuture().get(CI ? 10 : 2, SECONDS).asConnection(), instanceOf(HttpConnection.class)); } catch (ExecutionException e) { if (dryRun) { throw e; From c9f81b8f70672b7c6d51cd52116df485b209986b Mon Sep 17 00:00:00 2001 From: Bryce Anderson Date: Tue, 3 Dec 2024 08:41:30 -0700 Subject: [PATCH 06/13] try with resources --- ...rafficResilienceHttpServiceFilterTest.java | 93 ++++++++++--------- 1 file changed, 47 insertions(+), 46 deletions(-) diff --git a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java index 4694b9336e..1d225d23fd 100644 --- a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java +++ b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java @@ -231,61 +231,62 @@ void testStopAcceptingConnections(final boolean dryRun, final String protocol) t .dryRun(dryRun) .build(); - final HttpServerContext serverContext = HttpServers.forAddress(localAddress(0)) + try (HttpServerContext serverContext = HttpServers.forAddress(localAddress(0)) .protocols(protocolConfig) .listenSocketOption(SO_BACKLOG, TCP_BACKLOG) .appendNonOffloadingServiceFilter(filter) .listenStreamingAndAwait((ctx, request, responseFactory) -> - succeeded(responseFactory.ok().payloadBody(Publisher.never()))); + succeeded(responseFactory.ok().payloadBody(Publisher.never())))) { - final StreamingHttpClient client = HttpClients.forSingleAddress(serverHostAndPort(serverContext)) - .protocols(protocolConfig) - .socketOption(CONNECT_TIMEOUT, (int) SECONDS.toMillis(CI ? 4 : 2)) - .buildStreaming(); + try (StreamingHttpClient client = HttpClients.forSingleAddress(serverHostAndPort(serverContext)) + .protocols(protocolConfig) + .socketOption(CONNECT_TIMEOUT, (int) SECONDS.toMillis(CI ? 4 : 2)) + .buildStreaming()) { - // First request -> Pending 1 - final StreamingHttpRequest meta1 = client.newRequest(HttpRequestMethod.GET, "/"); - client.reserveConnection(meta1) - .flatMap(it -> it.request(meta1)) - .concat(Completable.defer(() -> { - // First request, has a "never" pub as a body, we don't attempt to consume it. - // Concat second request -> out of capacity -> server yielded - final StreamingHttpRequest meta2 = client.newRequest(HttpRequestMethod.GET, "/"); - return client.reserveConnection(meta2).flatMap(it -> it.request(meta2)).ignoreElement(); - })) - .toFuture() - .get(); + // First request -> Pending 1 + final StreamingHttpRequest meta1 = client.newRequest(HttpRequestMethod.GET, "/"); + client.reserveConnection(meta1) + .flatMap(it -> it.request(meta1)) + .concat(Completable.defer(() -> { + // First request, has a "never" pub as a body, we don't attempt to consume it. + // Concat second request -> out of capacity -> server yielded + final StreamingHttpRequest meta2 = client.newRequest(HttpRequestMethod.GET, "/"); + return client.reserveConnection(meta2).flatMap(it -> it.request(meta2)).ignoreElement(); + })) + .toFuture() + .get(); - // Netty will evaluate the "yielding" (i.e., auto-read) on this attempt, so this connection will go through. - assertThat(client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")) - .toFuture().get().asConnection(), instanceOf(HttpConnection.class)); + // Netty will evaluate the "yielding" (i.e., auto-read) on this attempt, so this connection will go + // through. + assertThat(client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")) + .toFuture().get().asConnection(), instanceOf(HttpConnection.class)); - // This connection shall full-fil the BACKLOG=1 setting - try { - System.out.println("Attempting to get another connection"); - assertThat(client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")) - // This is the failing line. - // https://github.com/apple/servicetalk/actions/runs/12129341561/job/33817567364?pr=3125 - // This now fails to resolve. - .toFuture().get(CI ? 10 : 2, SECONDS).asConnection(), instanceOf(HttpConnection.class)); - } catch (ExecutionException e) { - if (dryRun) { - throw e; - } - assertThat(e.getCause(), instanceOf(ConnectTimeoutException.class)); - } + // This connection shall full-fil the BACKLOG=1 setting + try { + assertThat(client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")) + // This is the failing line. + // https://github.com/apple/servicetalk/actions/runs/12129341561/job/33817567364?pr=3125 + // This now fails to resolve. + .toFuture().get(CI ? 10 : 2, SECONDS).asConnection(), instanceOf(HttpConnection.class)); + } catch (ExecutionException e) { + if (dryRun) { + throw e; + } + assertThat(e.getCause(), instanceOf(ConnectTimeoutException.class)); + } - System.out.println("Now we need to fail the connection."); - // Any attempt to create a connection now, should time out if we're not in dry mode. - if (dryRun) { - client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")).toFuture().get() - .releaseAsync().toFuture().get(); - } else { - try { - client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")).toFuture().get(); - fail("Expected a connection timeout"); - } catch (ExecutionException e) { - assertThat(e.getCause(), instanceOf(ConnectTimeoutException.class)); + // Any attempt to create a connection now, should time out if we're not in dry mode. + if (dryRun) { + client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")).toFuture().get() + .releaseAsync().toFuture().get(); + } else { + try { + client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")).toFuture().get(); + fail("Expected a connection timeout"); + } catch (ExecutionException e) { + assertThat(e.getCause(), instanceOf(ConnectTimeoutException.class)); + } + } } } } From 72ce45c8f8ad6b01881a42c1cfd73cd823613ad8 Mon Sep 17 00:00:00 2001 From: Bryce Anderson Date: Tue, 3 Dec 2024 09:25:30 -0700 Subject: [PATCH 07/13] rm unused import --- .../resilience/http/TrafficResilienceHttpServiceFilterTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java index 1d225d23fd..3b3e05ee3c 100644 --- a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java +++ b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java @@ -51,7 +51,6 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; -import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; import static io.netty.util.internal.PlatformDependent.normalizedOs; From fe56bdeb526a010dc884bd90338ebc9dad2e28b0 Mon Sep 17 00:00:00 2001 From: Bryce Anderson Date: Tue, 3 Dec 2024 10:58:14 -0700 Subject: [PATCH 08/13] bump iterations --- .../resilience/http/TrafficResilienceHttpServiceFilterTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java index 3b3e05ee3c..395854e85a 100644 --- a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java +++ b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java @@ -202,7 +202,7 @@ public TicketObserver onAllowedThrough(StreamingHttpRequest request, CapacityLim } @Test - @RepeatedTest(100) + @RepeatedTest(200) void repro() throws Exception { testStopAcceptingConnections(false, "h1"); } From c13990aeccaa28cba7e49cc08f9e0d318382686f Mon Sep 17 00:00:00 2001 From: Bryce Anderson Date: Tue, 3 Dec 2024 12:16:13 -0700 Subject: [PATCH 09/13] Cleanup --- .../http/TrafficResilienceHttpServiceFilterTest.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java index 395854e85a..d5e77b9ea5 100644 --- a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java +++ b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java @@ -202,7 +202,7 @@ public TicketObserver onAllowedThrough(StreamingHttpRequest request, CapacityLim } @Test - @RepeatedTest(200) + @RepeatedTest(100) void repro() throws Exception { testStopAcceptingConnections(false, "h1"); } @@ -263,10 +263,7 @@ void testStopAcceptingConnections(final boolean dryRun, final String protocol) t // This connection shall full-fil the BACKLOG=1 setting try { assertThat(client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")) - // This is the failing line. - // https://github.com/apple/servicetalk/actions/runs/12129341561/job/33817567364?pr=3125 - // This now fails to resolve. - .toFuture().get(CI ? 10 : 2, SECONDS).asConnection(), instanceOf(HttpConnection.class)); + .toFuture().get().asConnection(), instanceOf(HttpConnection.class)); } catch (ExecutionException e) { if (dryRun) { throw e; From f018c8e93eddb1e4c10140d2f0c5ae76b51e4086 Mon Sep 17 00:00:00 2001 From: Bryce Anderson Date: Tue, 3 Dec 2024 14:47:21 -0700 Subject: [PATCH 10/13] Just use a loop --- .../loadbalancer/DefaultLoadBalancer.java | 1 - ...rafficResilienceHttpServiceFilterTest.java | 40 ++++++++----------- 2 files changed, 17 insertions(+), 24 deletions(-) diff --git a/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancer.java b/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancer.java index f06273993e..14d6881677 100644 --- a/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancer.java +++ b/servicetalk-loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancer.java @@ -559,7 +559,6 @@ public Single newConnection(@Nullable final ContextMap context) { private Single selectConnection0(final Predicate selector, @Nullable final ContextMap context, final boolean forceNewConnectionAndReserve) { final HostSelector currentHostSelector = hostSelector; - LOGGER.info("Selecting connection."); Single result = currentHostSelector.selectConnection(selector, context, forceNewConnectionAndReserve); return result.beforeOnError(exn -> { if (exn instanceof NoActiveHostException) { diff --git a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java index d5e77b9ea5..b8d1763a0a 100644 --- a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java +++ b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java @@ -255,33 +255,27 @@ void testStopAcceptingConnections(final boolean dryRun, final String protocol) t .toFuture() .get(); - // Netty will evaluate the "yielding" (i.e., auto-read) on this attempt, so this connection will go - // through. - assertThat(client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")) - .toFuture().get().asConnection(), instanceOf(HttpConnection.class)); - - // This connection shall full-fil the BACKLOG=1 setting - try { - assertThat(client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")) - .toFuture().get().asConnection(), instanceOf(HttpConnection.class)); - } catch (ExecutionException e) { + // We expect up to a couple connections to succeed due to the intrinsic race between disabling accepts + // and new connect requests, as well as to account for kernel connect backlog. However, we do expect it + // to fail after a fairly short number of iterations. + for (int i = 0; i < 5; i++) { if (dryRun) { - throw e; + client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")).toFuture().get() + .releaseAsync().toFuture().get(); + } else { + try { + assertThat(client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")) + .toFuture().get().asConnection(), instanceOf(HttpConnection.class)); + } catch (ExecutionException e) { + assertThat(e.getCause(), instanceOf(ConnectTimeoutException.class)); + // We saw the connection rejection so we succeeded. + return; + } } - assertThat(e.getCause(), instanceOf(ConnectTimeoutException.class)); } - // Any attempt to create a connection now, should time out if we're not in dry mode. - if (dryRun) { - client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")).toFuture().get() - .releaseAsync().toFuture().get(); - } else { - try { - client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")).toFuture().get(); - fail("Expected a connection timeout"); - } catch (ExecutionException e) { - assertThat(e.getCause(), instanceOf(ConnectTimeoutException.class)); - } + if (!dryRun) { + fail("Connection was never rejected."); } } } From ad9f31847227aa65a1f924b149502a5d26ae670c Mon Sep 17 00:00:00 2001 From: Bryce Anderson Date: Tue, 3 Dec 2024 16:30:50 -0700 Subject: [PATCH 11/13] Remove test repeats --- .../http/TrafficResilienceHttpServiceFilterTest.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java index b8d1763a0a..fdce974c8e 100644 --- a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java +++ b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java @@ -44,7 +44,6 @@ import io.servicetalk.http.netty.HttpServers; import io.servicetalk.transport.api.ServerContext; -import org.junit.jupiter.api.RepeatedTest; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; @@ -201,12 +200,6 @@ public TicketObserver onAllowedThrough(StreamingHttpRequest request, CapacityLim assertThat(rejectedCount.get(), equalTo(1)); } - @Test - @RepeatedTest(100) - void repro() throws Exception { - testStopAcceptingConnections(false, "h1"); - } - @ParameterizedTest(name = "{displayName} [{index}] dryRun={0},protocol={1}") @CsvSource({"true, h1", "true, h2", "false, h1", "false, h2"}) void testStopAcceptingConnections(final boolean dryRun, final String protocol) throws Exception { From 3d400efbf34bbdb9bbfca93f9ac88020c1f049bd Mon Sep 17 00:00:00 2001 From: Bryce Anderson Date: Wed, 4 Dec 2024 12:05:22 -0700 Subject: [PATCH 12/13] More feedback --- .../http/TrafficResilienceHttpServiceFilterTest.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java index fdce974c8e..7ff3dd3b7f 100644 --- a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java +++ b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java @@ -56,7 +56,6 @@ import static io.servicetalk.capacity.limiter.api.CapacityLimiters.fixedCapacity; import static io.servicetalk.concurrent.api.Single.succeeded; import static io.servicetalk.concurrent.internal.DeliberateException.DELIBERATE_EXCEPTION; -import static io.servicetalk.concurrent.internal.TestTimeoutConstants.CI; import static io.servicetalk.http.api.HttpProtocolVersion.HTTP_1_1; import static io.servicetalk.http.netty.AsyncContextHttpFilterVerifier.verifyServerFilterAsyncContextVisibility; import static io.servicetalk.http.netty.HttpProtocolConfigs.h1Default; @@ -232,7 +231,7 @@ void testStopAcceptingConnections(final boolean dryRun, final String protocol) t try (StreamingHttpClient client = HttpClients.forSingleAddress(serverHostAndPort(serverContext)) .protocols(protocolConfig) - .socketOption(CONNECT_TIMEOUT, (int) SECONDS.toMillis(CI ? 4 : 2)) + .socketOption(CONNECT_TIMEOUT, (int) SECONDS.toMillis(2)) .buildStreaming()) { // First request -> Pending 1 @@ -240,7 +239,7 @@ void testStopAcceptingConnections(final boolean dryRun, final String protocol) t client.reserveConnection(meta1) .flatMap(it -> it.request(meta1)) .concat(Completable.defer(() -> { - // First request, has a "never" pub as a body, we don't attempt to consume it. + // The response has a "never" pub as a body and we don't attempt to consume it. // Concat second request -> out of capacity -> server yielded final StreamingHttpRequest meta2 = client.newRequest(HttpRequestMethod.GET, "/"); return client.reserveConnection(meta2).flatMap(it -> it.request(meta2)).ignoreElement(); @@ -251,7 +250,7 @@ void testStopAcceptingConnections(final boolean dryRun, final String protocol) t // We expect up to a couple connections to succeed due to the intrinsic race between disabling accepts // and new connect requests, as well as to account for kernel connect backlog. However, we do expect it // to fail after a fairly short number of iterations. - for (int i = 0; i < 5; i++) { + for (int i = 0; i < 3; i++) { if (dryRun) { client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")).toFuture().get() .releaseAsync().toFuture().get(); From 1fd70019dc38f70eecf9c81777c07d94c81e28ae Mon Sep 17 00:00:00 2001 From: Bryce Anderson Date: Wed, 4 Dec 2024 17:17:36 -0700 Subject: [PATCH 13/13] Clarify the reason for 3 iterations --- .../http/TrafficResilienceHttpServiceFilterTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java index 7ff3dd3b7f..9189d88e65 100644 --- a/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java +++ b/servicetalk-traffic-resilience-http/src/test/java/io/servicetalk/traffic/resilience/http/TrafficResilienceHttpServiceFilterTest.java @@ -248,8 +248,8 @@ void testStopAcceptingConnections(final boolean dryRun, final String protocol) t .get(); // We expect up to a couple connections to succeed due to the intrinsic race between disabling accepts - // and new connect requests, as well as to account for kernel connect backlog. However, we do expect it - // to fail after a fairly short number of iterations. + // and new connect requests, as well as to account for kernel connect backlog (effectively 1 for all + // OS's). That means we can have up to two connects succeed, but expect it to fail by the 3rd attempt. for (int i = 0; i < 3; i++) { if (dryRun) { client.reserveConnection(client.newRequest(HttpRequestMethod.GET, "/")).toFuture().get()