HTTP/2 downgrade to HTTP/1.1 not working

[jay]

I did this

Investigating #11034 which has a URL that when requested via HTTP/2 replies with RST_STREAM error HTTP_1_1_REQUIRED. libcurl should retry the transfer with HTTP/1.1 but doesn't. (Note the reporter in that issue builds libcurl without HTTP/2 so it is an unrelated issue).

curld -v https://wsnfse.vitoria.es.gov.br/producao/NotaFiscalService.asmx?WSDL

...
* HTTP/2 stream 1 was reset
* [CONN-0] readwrite_data() -> 56
* [CONN-0] Curl_readwrite() -> 56
* multi_done: status: 56 prem: 1 done: 0
* Connection #0 to host wsnfse.vitoria.es.gov.br left intact
* Expire cleared (transfer 0x526ee8)
curl: (56) HTTP/2 stream 1 was reset

Bisected to cab2d56 @icing

That commit changes http2_handle_stream_close to return a generic error before checking for CURLE_HTTP2_STREAM, which means CURLE_HTTP2_STREAM is never returned, the error is no longer properly output in verbose mode as HTTP_1_1_REQUIRED, and later code in libcurl to handle the downgrade is never reached:

curl/lib/http2.c

Lines 1579 to 1598 in 51f6a0d

	if(stream->error == NGHTTP2_REFUSED_STREAM) {
	DEBUGF(LOG_CF(data, cf, "[h2sid=%d] REFUSED_STREAM, try again on a new "
	"connection", stream->id));
	connclose(cf->conn, "REFUSED_STREAM"); /* don't use this anymore */
	data->state.refused_stream = TRUE;
	*err = CURLE_SEND_ERROR; /* trigger Curl_retry_request() later */
	return -1;
	}
	else if(stream->reset) {
	failf(data, "HTTP/2 stream %u was reset", stream->id);
	*err = stream->bodystarted? CURLE_PARTIAL_FILE : CURLE_RECV_ERROR;
	return -1;
	}
	else if(stream->error != NGHTTP2_NO_ERROR) {
	failf(data, "HTTP/2 stream %u was not closed cleanly: %s (err %u)",
	stream->id, nghttp2_http2_strerror(stream->error),
	stream->error);
	*err = CURLE_HTTP2_STREAM;
	return -1;
	}

curl/lib/multi.c

Lines 2479 to 2500 in 51f6a0d

	else if((CURLE_HTTP2_STREAM == result) &&
	Curl_h2_http_1_1_error(data)) {
	CURLcode ret = Curl_retry_request(data, &newurl);
	if(!ret) {
	infof(data, "Downgrades to HTTP/1.1");
	streamclose(data->conn, "Disconnect HTTP/2 for HTTP/1");
	data->state.httpwant = CURL_HTTP_VERSION_1_1;
	/* clear the error message bit too as we ignore the one we got */
	data->state.errorbuf = FALSE;
	if(!newurl)
	/* typically for HTTP_1_1_REQUIRED error on first flight */
	newurl = strdup(data->state.url);
	/* if we are to retry, set the result to OK and consider the request
	as done */
	retry = TRUE;
	result = CURLE_OK;
	done = TRUE;
	}
	else
	result = ret;
	}

It's unclear to me why else if(stream->reset) block needs to come before. Changing it back works but I don't know why it was changed in the first place there may be a good reason.

diff --git a/lib/http2.c b/lib/http2.c
index 9da3cae..0894561 100644
--- a/lib/http2.c
+++ b/lib/http2.c
@@ -1523,22 +1523,22 @@ static ssize_t http2_handle_stream_close(struct Curl_cfilter *cf,
     connclose(cf->conn, "REFUSED_STREAM"); /* don't use this anymore */
     data->state.refused_stream = TRUE;
     *err = CURLE_SEND_ERROR; /* trigger Curl_retry_request() later */
     return -1;
   }
-  else if(stream->reset) {
-    failf(data, "HTTP/2 stream %u was reset", stream->id);
-    *err = stream->bodystarted? CURLE_PARTIAL_FILE : CURLE_RECV_ERROR;
-    return -1;
-  }
   else if(stream->error != NGHTTP2_NO_ERROR) {
     failf(data, "HTTP/2 stream %u was not closed cleanly: %s (err %u)",
           stream->id, nghttp2_http2_strerror(stream->error),
           stream->error);
     *err = CURLE_HTTP2_STREAM;
     return -1;
   }
+  else if(stream->reset) {
+    failf(data, "HTTP/2 stream %u was reset", stream->id);
+    *err = stream->bodystarted? CURLE_PARTIAL_FILE : CURLE_RECV_ERROR;
+    return -1;
+  }
 
   if(!stream->bodystarted) {
     failf(data, "HTTP/2 stream %u was closed cleanly, but before getting "
           " all response header fields, treated as error",
           stream->id);

/cc @icing

[bagder]

@icing this feels like something we should be able to add a test for these days, right?

[icing]

Added #11362 in the now more depressing github UI.