POST h2c (http) with HTTP/2

Author: 3dgiordano

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

@@ -863,6 +863,15 @@ public int getRequestTimeout() {
     return requestTimeout;
   }
 
+  /**
+   * Whether a {@code protocol_error} may be retried over HTTP/1.1 with this client's resolved
+   * configuration. Already {@code false} whenever HTTP/1.1 is disabled, so callers do not have to
+   * pair it with a separate HTTP/1.1 check.
+   */
+  public boolean isProtocolErrorFallbackEnabled() {
+    return protocolErrorFallbackEnabled;
+  }
+
   public void loadProperties() {
     loadProperties(null);
   }
@@ -4003,13 +4012,44 @@ private HttpClient resolveClientForRequest(HTTP2Sampler sampler, HTTPSampleResul
       throws URISyntaxException {
     URI uri = result.getURL().toURI();
     if ("http".equalsIgnoreCase(uri.getScheme())
-        && shouldAttachRequestBody(sampler, result, false)) {
+        && shouldAttachRequestBody(sampler, result, false)
+        && canDivertCleartextBodyToHttp11(uri)) {
       lowLevelDebug("Cleartext request with body; using HTTP/1.1-only client for {}", uri);
       return httpClientHttp1Only;
     }
     return selectHttpClient(uri, isRecoverableIfHttp3Fails(sampler));
   }
 
+  /**
+   * Whether a bodied cleartext request may be diverted to the HTTP/1.1-only client.
+   *
+   * <p>The diversion only exists to sidestep the h2c Upgrade dance, whose first request travels as
+   * plain HTTP/1.1 and which servers handle inconsistently when it carries a body. It is a
+   * shortcut around a negotiation, never a protocol choice, so it must not fire where HTTP/1.1 is
+   * not what the configuration asks for:
+   *
+   * <ul>
+   *   <li>HTTP/1.1 disabled: no request may go out as HTTP/1.1, bodied or not. Sending one to an
+   *       h2c origin makes the server answer with HTTP/2 frames that the HTTP/1.1 parser reads as
+   *       garbage ({@code Illegal character CNTL=0x0}).</li>
+   *   <li>h2c prior knowledge (configured, or learned and still cached): the origin is spoken to
+   *       as HTTP/2 from the first byte, so there is no Upgrade to avoid in the first place.</li>
+   * </ul>
+   */
+  private boolean canDivertCleartextBodyToHttp11(URI uri) {
+    if (!enableHttp1) {
+      lowLevelDebug("Cleartext request with body but HTTP/1.1 is disabled; "
+          + "keeping protocol selection for {}", uri);
+      return false;
+    }
+    if (shouldUseH2cPriorKnowledge(uri)) {
+      lowLevelDebug("Cleartext request with body on an h2c prior-knowledge origin; "
+          + "keeping HTTP/2 for {}", uri);
+      return false;
+    }
+    return true;
+  }
+
   /**
    * Whether a failed HTTP/3 attempt for this request could still be served over another protocol.
    * Used when deciding fallback after an indicated HTTP/3 failure (cached Alt-Svc / prior

src/main/java/com/blazemeter/jmeter/http2/sampler/HTTP2Sampler.java

@@ -476,25 +476,31 @@ protected HTTPSampleResult sample(URL url, String method, boolean areFollowingRe
       if ((isProtocolErrorCause || isProtocolErrorException)
           && !HpackFailureDetector.indicatesHpackFailure(e)
           && !HpackFailureDetector.indicatesHpackFailure(cause)) {
-        boolean fallbackEnabled = isProtocolErrorFallbackEnabled();
+        HTTP2JettyClient fallbackClient = resolveClientForFallback();
+        // The client owns the resolved answer: it already merges this sampler's own flags with
+        // the JMeter properties, and it already turns the fallback off when HTTP/1.1 is disabled.
+        // Re-deriving it from properties alone here ignored both, and retried over HTTP/1.1 even
+        // against h2c-only endpoints where no request may go out as HTTP/1.1.
+        boolean fallbackEnabled =
+            fallbackClient != null && fallbackClient.isProtocolErrorFallbackEnabled();
         if (!fallbackEnabled) {
           LOG.warn("HTTP/2 protocol_error detected and fallback is DISABLED. "
               + "Request will fail.");
           LOG.warn("Error: {}", cause != null ? cause.getMessage() : e.getMessage());
           LOG.warn("To enable fallback, set blazemeter.http.protocolErrorFallbackEnabled=true "
-              + "or blazemeter.http.disableFallback=false in user.properties or jmeter.properties");
+              + "or blazemeter.http.disableFallback=false in user.properties or jmeter.properties, "
+              + "and keep HTTP/1.1 enabled on the sampler");
         } else {
           LOG.warn("HTTP/2 protocol_error detected. Attempting fallback to HTTP/1.1");
           LOG.warn("Error: {}", cause != null ? cause.getMessage() : e.getMessage());
 
           try {
-            // Get the client and request details for fallback
-            HTTP2JettyClient client = clientFactory.call();
             HTTPSampleResult fallbackBase = resolveErrorResult(preparedResult, url, method);
 
             // Retry with HTTP/1.1 only
             LOG.info("Retrying request with HTTP/1.1 only: {}", url);
-            HTTPSampleResult fallbackResult = client.retryWithHTTP11Only(this, fallbackBase);
+            HTTPSampleResult fallbackResult =
+                fallbackClient.retryWithHTTP11Only(this, fallbackBase);
 
             if (fallbackResult != null && fallbackResult.isSuccessful()) {
               LOG.info("HTTP/1.1 fallback succeeded: status={}", fallbackResult.getResponseCode());
@@ -523,17 +529,17 @@ private HTTPSampleResult resolveErrorResult(HTTPSampleResult preparedResult, URL
     return buildResult(url, method);
   }
 
-  private boolean isProtocolErrorFallbackEnabled() {
-    String fb =
-        BzmHttpPluginProperties.resolveRaw("httpJettyClient.protocolErrorFallbackEnabled");
-    if (fb != null) {
-      return Boolean.parseBoolean(fb);
-    }
-    String df = BzmHttpPluginProperties.resolveRaw("httpJettyClient.disableFallback");
-    if (df != null) {
-      return !Boolean.parseBoolean(df);
+  /**
+   * The client that would serve this sampler, or {@code null} when it cannot be obtained. Used
+   * from the failure path, where losing the client means there is nothing left to retry on.
+   */
+  private HTTP2JettyClient resolveClientForFallback() {
+    try {
+      return clientFactory.call();
+    } catch (Exception e) {
+      LOG.error("Could not obtain the client to evaluate the HTTP/1.1 fallback", e);
+      return null;
     }
-    return true;
   }
 
   protected Request sampleAsync(HTTPSampleResult result, HTTP2FutureResponseListener listener)

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

@@ -0,0 +1,129 @@
+package com.blazemeter.jmeter.http2.core;
+
+import static org.junit.Assert.assertSame;
+
+import com.blazemeter.jmeter.http2.HTTP2TestBase;
+import com.blazemeter.jmeter.http2.sampler.HTTP2Sampler;
+import com.blazemeter.jmeter.http2.sampler.JMeterTestUtils;
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+import java.net.URI;
+import org.apache.jmeter.protocol.http.sampler.HTTPSampleResult;
+import org.apache.jmeter.protocol.http.util.HTTPConstants;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/**
+ * Pins which client a bodied cleartext request gets.
+ *
+ * <p>Such requests are diverted to the HTTP/1.1-only client to sidestep the h2c Upgrade dance,
+ * whose first request travels as plain HTTP/1.1 and which servers handle inconsistently when it
+ * carries a body. That is a shortcut around a negotiation, not a protocol choice, so it must not
+ * survive where HTTP/1.1 is disabled or where h2c is spoken from the first byte anyway — issue
+ * #157, where POSTs to an h2c-only endpoint went out as HTTP/1.1 and the server's HTTP/2 frames
+ * came back through the HTTP/1.1 parser as {@code Illegal character CNTL=0x0}.
+ */
+public class CleartextBodyClientSelectionTest extends HTTP2TestBase {
+
+  private static final URI CLEARTEXT_URI = URI.create("http://h2c-only.example.com:8000/policies");
+
+  private HTTP2JettyClient client;
+
+  @BeforeClass
+  public static void setupClass() {
+    JMeterTestUtils.setupJmeterEnv();
+  }
+
+  @Before
+  public void setUp() throws Exception {
+    HTTP2JettyClient.clearStaticProtocolCaches();
+    client = new HTTP2JettyClient();
+    setBoolean("enableHttp1", true);
+    setBoolean("enableHttp2", true);
+    setBoolean("enableHttp3", false);
+    setBoolean("http1UpgradeRequired", false);
+    setBoolean("http2PriorKnowledgeEnabled", false);
+    setBoolean("http3PriorKnowledgeEnabled", false);
+    setBoolean("h2cCacheEnabled", false);
+  }
+
+  @Test
+  public void shouldUseHttp1OnlyForCleartextPostWhenUpgradeIsTheAlternative() throws Exception {
+    setBoolean("http1UpgradeRequired", true);
+
+    assertSame("a bodied cleartext POST must skip the h2c Upgrade dance",
+        field("httpClientHttp1Only"), resolveClientForRequest(post()));
+  }
+
+  @Test
+  public void shouldNotUseHttp1OnlyForCleartextPostWhenHttp1IsDisabled() throws Exception {
+    setBoolean("enableHttp1", false);
+    setBoolean("http1UpgradeRequired", true);
+
+    assertSame("with HTTP/1.1 disabled a bodied POST must still be spoken as h2c",
+        field("httpClientH2cPrior"), resolveClientForRequest(post()));
+  }
+
+  @Test
+  public void shouldNotUseHttp1OnlyForCleartextPostWithH2cPriorKnowledge() throws Exception {
+    setBoolean("http2PriorKnowledgeEnabled", true);
+
+    assertSame("prior knowledge means there is no Upgrade to avoid",
+        field("httpClientH2cPrior"), resolveClientForRequest(post()));
+  }
+
+  @Test
+  public void shouldUseHttp1OnlyForCleartextGetSendingParametersAsBody() throws Exception {
+    setBoolean("http1UpgradeRequired", true);
+    HTTP2Sampler sampler = sampler(HTTPConstants.GET);
+    sampler.setPostBodyRaw(true);
+
+    assertSame(field("httpClientHttp1Only"),
+        resolveClientForRequest(sampler, result(HTTPConstants.GET)));
+  }
+
+  private HTTP2Sampler post() {
+    HTTP2Sampler sampler = sampler(HTTPConstants.POST);
+    sampler.addArgument("policy", "value");
+    return sampler;
+  }
+
+  private HTTP2Sampler sampler(String method) {
+    HTTP2Sampler sampler = new HTTP2Sampler();
+    sampler.setMethod(method);
+    return sampler;
+  }
+
+  private HTTPSampleResult result(String method) throws Exception {
+    HTTPSampleResult result = new HTTPSampleResult();
+    result.setURL(CLEARTEXT_URI.toURL());
+    result.setHTTPMethod(method);
+    return result;
+  }
+
+  private Object resolveClientForRequest(HTTP2Sampler sampler) throws Exception {
+    return resolveClientForRequest(sampler, result(HTTPConstants.POST));
+  }
+
+  private Object resolveClientForRequest(HTTP2Sampler sampler, HTTPSampleResult result)
+      throws Exception {
+    Method m = HTTP2JettyClient.class.getDeclaredMethod(
+        "resolveClientForRequest", HTTP2Sampler.class, HTTPSampleResult.class);
+    m.setAccessible(true);
+    return m.invoke(client, sampler, result);
+  }
+
+  private Object field(String name) throws Exception {
+    Field f = HTTP2JettyClient.class.getDeclaredField(name);
+    f.setAccessible(true);
+    return f.get(client);
+  }
+
+  private void setBoolean(String name, boolean value) throws Exception {
+    Field f = HTTP2JettyClient.class.getDeclaredField(name);
+    f.setAccessible(true);
+    f.setBoolean(client, value);
+  }
+
+}