internal/http3: more robust handling of request & response with no body

In HTTP/3, zero or more DATA frames can come after a HEADERS frame to
represent a request or response body. Our current implementation can
behave rather badly when zero DATA frame is sent.

ClientConn does not close the write direction of the stream when it has
no body to send. As a result, our Server can end up reading the next
frame after a HEADERS frame, only to hang infinitely until the timeout
is reached. To fix this, when there is no body to send, ClientConn now
closes the write direction of the stream as soon as it has finished
writing its HEADERS frame. Server will also prevent itself from reading
the stream if a Content-Length header with the value 0 is received.

In the opposite direction (client reading response from a server), a
similar problem also exists, with a slight variant. While our Server
reliably closes its write direction of the stream as soon as the server
handler exits, a problem can still occur when a client receives an empty
response body due to sending a HEAD request. In this case, if the client
decides to read the response body, bodyReader might throw an error due
to a mismatch between the Content-Length header given by the server and
the actual body length. This is fixed by making ClientConn aware that
HEAD requests will always result in an empty response body.

For golang/go#70914

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

internal/http3/roundtrip.go

@@ -26,7 +26,7 @@ type roundTripState struct {
 	reqBodyWriter bodyWriter
 
 	// Response.Body, provided to the caller.
-	respBody bodyReader
+	respBody io.ReadCloser
 
 	errOnce sync.Once
 	err     error
@@ -126,6 +126,11 @@ func (cc *ClientConn) RoundTrip(req *http.Request) (_ *http.Response, err error)
 			encr.HasBody = false
 			go copyRequestBody(rt)
 		}
+	} else {
+		// If we have no body to send, close the write direction of the stream
+		// as soon as we have sent our HEADERS. That way, servers will know
+		// that there are no DATA frames incoming.
+		rt.st.stream.CloseWrite()
 	}
 
 	// Read the response headers.
@@ -164,8 +169,14 @@ func (cc *ClientConn) RoundTrip(req *http.Request) (_ *http.Response, err error)
 			if err != nil {
 				return nil, err
 			}
-			rt.respBody.st = st
-			rt.respBody.remain = contentLength
+			if contentLength != 0 && req.Method != http.MethodHead {
+				rt.respBody = &bodyReader{
+					st:     st,
+					remain: contentLength,
+				}
+			} else {
+				rt.respBody = http.NoBody
+			}
 			resp := &http.Response{
 				Proto:         "HTTP/3.0",
 				ProtoMajor:    3,

internal/http3/roundtrip_test.go

@@ -419,3 +419,63 @@ func TestRoundTripExpect100ContinueRejected(t *testing.T) {
 		rt.wantBody(serverBody)
 	})
 }
+
+func TestRoundTripNoBodyClosesStream(t *testing.T) {
+	synctest.Test(t, func(t *testing.T) {
+		tc := newTestClientConn(t)
+		tc.greet()
+
+		req, _ := http.NewRequest("PUT", "https://example.tld/", nil)
+		tc.roundTrip(req)
+		st := tc.wantStream(streamTypeRequest)
+
+		st.wantHeaders(nil)
+		st.wantClosed("no DATA frames to send")
+	})
+}
+
+func TestRoundTripReadRespWithNoBody(t *testing.T) {
+	synctest.Test(t, func(t *testing.T) {
+		tc := newTestClientConn(t)
+		tc.greet()
+
+		// Case 1: we know response body is empty because the server closes the
+		// write direction of the stream.
+		req, _ := http.NewRequest("GET", "https://example.tld/", nil)
+		rt := tc.roundTrip(req)
+		st := tc.wantStream(streamTypeRequest)
+		st.wantHeaders(nil)
+		st.writeHeaders(http.Header{
+			":status": {"200"},
+		})
+		st.stream.stream.CloseWrite()
+		rt.wantStatus(200)
+		st.wantClosed("request is complete")
+
+		// Case 2: we know response body is empty because the server indicates
+		// a Content-Length of 0.
+		req, _ = http.NewRequest("GET", "https://example.tld/", nil)
+		rt = tc.roundTrip(req)
+		st = tc.wantStream(streamTypeRequest)
+		st.wantHeaders(nil)
+		st.writeHeaders(http.Header{
+			":status":        {"200"},
+			"Content-Length": {"0"},
+		})
+		rt.wantStatus(200)
+		st.wantClosed("request is complete")
+
+		// Case 3: we know response body is empty because we sent a HEAD
+		// request.
+		req, _ = http.NewRequest("HEAD", "https://example.tld/", nil)
+		rt = tc.roundTrip(req)
+		st = tc.wantStream(streamTypeRequest)
+		st.wantHeaders(nil)
+		st.writeHeaders(http.Header{
+			":status":        {"200"},
+			"Content-Length": {"1000"},
+		})
+		rt.wantStatus(200)
+		st.wantClosed("request is complete")
+	})
+}

internal/http3/server.go

@@ -6,6 +6,7 @@ package http3
 
 import (
 	"context"
+	"io"
 	"net/http"
 	"strconv"
 	"sync"
@@ -217,6 +218,21 @@ func (sc *serverConn) handleRequestStream(st *stream) error {
 			message: reqInfo.InvalidReason,
 		}
 	}
+
+	var body io.ReadCloser
+	contentLength := int64(-1)
+	if n, err := strconv.Atoi(header.Get("Content-Length")); err == nil {
+		contentLength = int64(n)
+	}
+	if contentLength != 0 {
+		body = &bodyReader{
+			st:     st,
+			remain: contentLength,
+		}
+	} else {
+		body = http.NoBody
+	}
+
 	req := &http.Request{
 		Proto:      "HTTP/3.0",
 		Method:     pHeader.method,
@@ -226,11 +242,8 @@ func (sc *serverConn) handleRequestStream(st *stream) error {
 		Trailer:    reqInfo.Trailer,
 		ProtoMajor: 3,
 		RemoteAddr: sc.qconn.RemoteAddr().String(),
-		Body: &bodyReader{
-			st:     st,
-			remain: -1,
-		},
-		Header: header,
+		Body:       body,
+		Header:     header,
 	}
 	defer req.Body.Close()
 

internal/http3/server_test.go

@@ -373,6 +373,41 @@ func TestServerExpect100ContinueRejected(t *testing.T) {
 	})
 }
 
+func TestServerHandlerReadReqWithNoBody(t *testing.T) {
+	synctest.Test(t, func(t *testing.T) {
+		serverBody := []byte("hello from server!")
+		ts := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			if _, err := io.ReadAll(r.Body); err != nil {
+				t.Errorf("got %v err when reading from an empty request body, want nil", err)
+			}
+			w.Write(serverBody)
+		}))
+		tc := ts.connect()
+		tc.greet()
+
+		// Case 1: we know that there is no body / DATA frame because the
+		// client closes the write direction of the stream.
+		reqStream := tc.newStream(streamTypeRequest)
+		reqStream.writeHeaders(requestHeader(nil))
+		reqStream.stream.stream.CloseWrite()
+		synctest.Wait()
+		reqStream.wantHeaders(http.Header{":status": {"200"}})
+		reqStream.wantData(serverBody)
+		reqStream.wantClosed("request is complete")
+
+		// Case 2: we know that there is no body / DATA frame because the
+		// client indicates a Content-Length of 0.
+		reqStream = tc.newStream(streamTypeRequest)
+		reqStream.writeHeaders(requestHeader(http.Header{
+			"Content-Length": {"0"},
+		}))
+		synctest.Wait()
+		reqStream.wantHeaders(http.Header{":status": {"200"}})
+		reqStream.wantData(serverBody)
+		reqStream.wantClosed("request is complete")
+	})
+}
+
 type testServer struct {
 	t  testing.TB
 	s  *Server