Add H2C fallback to HTTP/1.1 support

src/main/java/com/blazemeter/jmeter/http2/core/HTTP2JettyClient.java

@@ -189,6 +189,17 @@ public class HTTP2JettyClient {
   private static final String ATTR_HTTP3_ATTEMPTED = "bzm.http3.attempted";
   private static final String ATTR_H2C_FALLBACK_ATTEMPTED = "bzm.h2cFallbackAttempted";
   private static final String ATTR_SKIP_H2C_UPGRADE = "bzm.skipH2cUpgrade";
+  /**
+   * Statuses that answer an {@code Upgrade: h2c} attempt by refusing it rather than by serving the
+   * request: a bad request, an upgrade demanded or not implemented, a version not supported. Only
+   * these are worth sending again without the upgrade headers - see
+   * {@link #shouldRetryAfterFailedH2cUpgrade}.
+   */
+  private static final Set<Integer> H2C_UPGRADE_REFUSED_STATUSES = Set.of(
+      HttpStatus.BAD_REQUEST_400,
+      HttpStatus.UPGRADE_REQUIRED_426,
+      HttpStatus.NOT_IMPLEMENTED_501,
+      HttpStatus.HTTP_VERSION_NOT_SUPPORTED_505);
   private static final String ATTR_ORIGIN_KEY = "bzm.http3.origin";
   private static final String ATTR_REQUEST_HEADERS_SERIALIZED = "bzm.request.headers.serialized";
   /**
@@ -1450,9 +1461,20 @@ private boolean wasH2cUpgradeAttempt(Request request) {
   }
 
   /**
-   * The server answered (no timeout/error) but never actually negotiated HTTP/2 despite our
-   * {@code Upgrade: h2c} attempt - e.g. it silently ignored the header, as a compliant HTTP/1.1
-   * server that doesn't support h2c is allowed to do. Retry once with a plain HTTP/1.1 request.
+   * Whether the {@code Upgrade: h2c} attempt has to be made again as a plain HTTP/1.1 request.
+   *
+   * <p>Only when the answer says the upgrade attempt itself was refused. A compliant HTTP/1.1
+   * server that does not speak h2c ignores the header and serves the request over HTTP/1.1, and
+   * that response <em>is</em> the answer to this request: re-sending it would put the same request
+   * on the wire twice - twice the load, and twice the side effect for anything that is not a GET -
+   * and would report only the second attempt's time. What is left of the failed negotiation is
+   * remembered by {@link #updateHttp1OnlyCache}, so the requests after it skip the upgrade instead
+   * of paying for it again.
+   *
+   * <p>A server or proxy that answers the upgrade headers with one of
+   * {@link #H2C_UPGRADE_REFUSED_STATUSES} did not serve the request, it rejected the attempt, and
+   * that is a failure this client caused by adding headers the test plan never asked for. Those are
+   * retried once, without the headers.
    */
   private boolean shouldRetryAfterFailedH2cUpgrade(Request request, ContentResponse response) {
     if (!enableHttp1 || !http1UpgradeRequired || request == null || response == null) {
@@ -1469,7 +1491,8 @@ private boolean shouldRetryAfterFailedH2cUpgrade(Request request, ContentRespons
     if (!wasH2cUpgradeAttempt(request)) {
       return false;
     }
-    return response.getVersion() != HttpVersion.HTTP_2;
+    return response.getVersion() != HttpVersion.HTTP_2
+        && H2C_UPGRADE_REFUSED_STATUSES.contains(response.getStatus());
   }
 
   private void markCleartextHttp1Only(URI uri) {
@@ -4196,8 +4219,14 @@ private void setHeaders(Request request, URL url, HeaderManager headerManager) {
     // 1. The connection is already HTTP/2 (negotiated via ALPN)
     // 2. Upgrade headers are for cleartext HTTP, not HTTPS
     // 3. It violates the HTTP/2 protocol (RFC 7540)
+    // An origin already known to answer HTTP/1.1 is not asked to upgrade again: the attempt costs
+    // three headers and Jetty's upgrade machinery on every request, and the negotiation it asks for
+    // is one this client already watched fail. This is the "later requests skip the futile upgrade
+    // attempt" the HTTP/1.1-only cache is written for - until now the cache only steered which
+    // client was used, and the headers went out regardless.
     if (http1UpgradeRequired && enableHttp2 && !"https".equalsIgnoreCase(url.getProtocol())
         && !shouldUseH2cPriorKnowledge(request.getURI())
+        && !isHttp1Only(request.getURI())
         && !Boolean.TRUE.equals(request.getAttributes().get(ATTR_SKIP_H2C_UPGRADE))) {
       Mutable headers = ((Mutable) request.getHeaders());
       addHeaderIfMissing(HttpHeader.UPGRADE, "h2c", headers);

src/test/java/com/blazemeter/jmeter/http2/core/HTTP2JettyClientH2cFallbackTest.java

@@ -1,6 +1,8 @@
 package com.blazemeter.jmeter.http2.core;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 import com.blazemeter.jmeter.http2.HTTP2TestBase;
 import com.blazemeter.jmeter.http2.core.ServerBuilder.TeardownableServer;
@@ -10,7 +12,9 @@
 import java.net.URL;
 import java.nio.channels.ClosedChannelException;
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.atomic.AtomicInteger;
 import org.apache.jmeter.protocol.http.sampler.HTTPSampleResult;
+import org.eclipse.jetty.client.ContentResponse;
 import org.eclipse.jetty.client.HttpClient;
 import org.eclipse.jetty.client.Request;
 import org.eclipse.jetty.http.HttpFields;
@@ -81,6 +85,81 @@ public void secondRequestToSameOriginSkipsRepeatedFailedH2cUpgrade() throws Exce
     }
   }
 
+  /**
+   * A server that ignores {@code Upgrade: h2c} and answers the request over HTTP/1.1 has answered
+   * it: that response is the sample. Re-sending would hit the server twice for one sampler, which
+   * for anything other than a GET means the side effect happens twice, and would report only the
+   * second attempt's time.
+   */
+  @Test
+  public void anUpgradeAttemptAnsweredOverHttp11PutsOneRequestOnTheWire() throws Exception {
+    AtomicInteger requests = new AtomicInteger();
+    int port = startHttp1OnlyServer(requests);
+    // http1UpgradeRequired=true is what makes this request carry the h2c upgrade headers.
+    HTTP2JettyClient client = new HTTP2JettyClient(true, "h2c-single-request-test");
+    client.start();
+    try {
+      HTTPSampleResult result = sampleGet(client, port);
+
+      assertThat(result.isSuccessful()).isTrue();
+      assertThat(result.getResponseCode()).isEqualTo("200");
+      assertThat(requests.get()).as("requests that reached the server").isEqualTo(1);
+    } finally {
+      client.stop();
+    }
+  }
+
+  /** And the origin is remembered, so the requests after it are not upgrade attempts either. */
+  @Test
+  public void furtherRequestsToAnHttp11OriginPutOneRequestEachOnTheWire() throws Exception {
+    AtomicInteger requests = new AtomicInteger();
+    int port = startHttp1OnlyServer(requests);
+    HTTP2JettyClient client = new HTTP2JettyClient(true, "h2c-single-request-cache-test");
+    client.start();
+    try {
+      sampleGet(client, port);
+      sampleGet(client, port);
+      sampleGet(client, port);
+
+      assertThat(requests.get()).as("requests that reached the server for three samplers")
+          .isEqualTo(3);
+    } finally {
+      client.stop();
+    }
+  }
+
+  /**
+   * The other half of the rule: a server or proxy that answers the upgrade headers by refusing them
+   * did not serve the request, and that failure is one this client caused by adding headers the test
+   * plan never asked for. Those are still sent again without them.
+   */
+  @Test
+  public void onlyAnAnswerThatRefusesTheUpgradeIsSentAgain() throws Exception {
+    HTTP2JettyClient client = new HTTP2JettyClient(true, "h2c-retry-decision-test");
+    Request upgradeAttempt = newProbeRequest("http://example.invalid/")
+        .headers(h -> h.put(HttpHeader.UPGRADE, "h2c"));
+
+    for (int refused : new int[] {400, 426, 501, 505}) {
+      assertThat(shouldRetry(client, upgradeAttempt, refused))
+          .as("status %s refuses the upgrade, so the request must be sent again", refused)
+          .isTrue();
+    }
+    for (int served : new int[] {200, 201, 204, 301, 401, 403, 404, 500, 503}) {
+      assertThat(shouldRetry(client, upgradeAttempt, served))
+          .as("status %s is an answer to the request, so it must not be sent again", served)
+          .isFalse();
+    }
+  }
+
+  private static boolean shouldRetry(HTTP2JettyClient client, Request request, int status)
+      throws Exception {
+    ContentResponse response = mock(ContentResponse.class);
+    when(response.getVersion()).thenReturn(HttpVersion.HTTP_1_1);
+    when(response.getStatus()).thenReturn(status);
+    return (Boolean) invokePrivate(client, "shouldRetryAfterFailedH2cUpgrade",
+        new Class<?>[] {Request.class, ContentResponse.class}, request, response);
+  }
+
   @Test
   public void wasH2cUpgradeAttemptDetectsUpgradeHeader() throws Exception {
     HTTP2JettyClient client = newClientForReflection();
@@ -132,7 +211,17 @@ public void isClosedChannelFailureFindsItAnywhereInCauseChain() throws Exception
   }
 
   private int startHttp1OnlyServer() throws Exception {
+    return startHttp1OnlyServer(null);
+  }
+
+  /**
+   * @param requestCounter counts every request the server answered, or {@code null} to not count
+   */
+  private int startHttp1OnlyServer(AtomicInteger requestCounter) throws Exception {
     server = new ServerBuilder().withHTTP1().buildServer();
+    if (requestCounter != null) {
+      server.setRequestLog((request, response) -> requestCounter.incrementAndGet());
+    }
     server.start();
     return ((ServerConnector) server.getConnectors()[0]).getLocalPort();
   }