http2: do not retry RoundTrip after peer sends a stream protocol error

Years back, our HTTP/2 server implementation had a stream accounting bug
that would cause it to improperly report a PROTOCOL_ERROR. In response
to this, we modified our Transport to retry RoundTrip when a RST_STREAM
with PROTOCOL_ERROR was received from a peer.

At this point, this retry logic had outlived its usefulness. Instead, it
might cause issues, e.g. a client that sends a malformed request will
keep retrying repeatedly, despite there being zero chance for the
request to actually succeed.

Fixes golang/go#77843

Change-Id: Ic043723e3535f68f91db33d8f6bcd7fc2dbce856
Reviewed-on: https://go-review.googlesource.com/c/net/+/750720
Reviewed-by: Nicholas Husin <husin@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

Author: nicholashusin

http2/transport.go

@@ -712,10 +712,6 @@ func canRetryError(err error) bool {
 		return true
 	}
 	if se, ok := err.(StreamError); ok {
-		if se.Code == ErrCodeProtocol && se.Cause == errFromPeer {
-			// See golang/go#47635, golang/go#42777
-			return true
-		}
 		return se.Code == ErrCodeRefusedStream
 	}
 	return false

http2/transport_test.go

@@ -4838,15 +4838,15 @@ func TestTransportCloseRequestBody(t *testing.T) {
 	}
 }
 
-func TestTransportRetriesOnStreamProtocolError(t *testing.T) {
-	synctestTest(t, testTransportRetriesOnStreamProtocolError)
+func TestTransportNoRetryOnStreamProtocolError(t *testing.T) {
+	synctestTest(t, testTransportNoRetryOnStreamProtocolError)
 }
-func testTransportRetriesOnStreamProtocolError(t testing.TB) {
-	// This test verifies that
+func testTransportNoRetryOnStreamProtocolError(t testing.TB) {
+	// This test verifies that:
+	//   - a request that fails with ErrCodeProtocol is not retried. See
+	//     go.dev/issue/77843.
 	//   - receiving a protocol error on a connection does not interfere with
-	//     other requests in flight on that connection;
-	//   - the connection is not reused for further requests; and
-	//   - the failed request is retried on a new connecection.
+	//     other requests in flight on that connection.
 	tt := newTestTransport(t)
 
 	// Start two requests. The first is a long request
@@ -4866,44 +4866,26 @@ func testTransportRetriesOnStreamProtocolError(t testing.TB) {
 	tc1.writeSettings()
 	tc1.wantFrameType(FrameSettings) // settings ACK
 
-	// Request #2(a): The short request.
+	// Request #2: The short request.
 	req2, _ := http.NewRequest("GET", "https://dummy.tld/", nil)
 	rt2 := tt.roundTrip(req2)
 	tc1.wantHeaders(wantHeader{
 		streamID:  3,
 		endStream: true,
 	})
 
-	// Request #2(a) fails with ErrCodeProtocol.
+	// Request #2 fails with ErrCodeProtocol.
 	tc1.writeRSTStream(3, ErrCodeProtocol)
 	if rt1.done() {
 		t.Fatalf("After protocol error on RoundTrip #2, RoundTrip #1 is done; want still in progress")
 	}
-	if rt2.done() {
-		t.Fatalf("After protocol error on RoundTrip #2, RoundTrip #2 is done; want still in progress")
+	if !rt2.done() {
+		t.Fatalf("After protocol error on RoundTrip #2, RoundTrip #2 is in progress; want done")
+	}
+	// Request #2 should not be retried.
+	if tt.hasConn() {
+		t.Fatalf("After protocol error on RoundTrip #2, RoundTrip #2 is unexpectedly retried")
 	}
-
-	// Request #2(b): The short request is retried on a new connection.
-	tc2 := tt.getConn()
-	tc2.wantFrameType(FrameSettings)
-	tc2.wantFrameType(FrameWindowUpdate)
-	tc2.wantHeaders(wantHeader{
-		streamID:  1,
-		endStream: true,
-	})
-	tc2.writeSettings()
-	tc2.wantFrameType(FrameSettings) // settings ACK
-
-	// Request #2(b) succeeds.
-	tc2.writeHeaders(HeadersFrameParam{
-		StreamID:   1,
-		EndHeaders: true,
-		EndStream:  true,
-		BlockFragment: tc1.makeHeaderBlockFragment(
-			":status", "201",
-		),
-	})
-	rt2.wantStatus(201)
 
 	// Request #1 succeeds.
 	tc1.writeHeaders(HeadersFrameParam{