[net] internal/http3: add Expect: 100-continue support to ClientConn
Nicholas Husin (Gerrit)
Nicholas Husin has uploaded the change for review![]( "Open in Gerrit")
Nicholas Husin would like Damien Neil to review this change.
Commit message
internal/http3: add Expect: 100-continue support to ClientConn
When sending a request containing the "Expect: 100-continue" header,
ClientConn.RoundTrip will now only send the request body after receiving
an HTTP 100 status response from the server.
For golang/go#70914
Change diff
diff --git a/internal/http3/roundtrip.go b/internal/http3/roundtrip.go
index d52c845..8c7e262 100644
--- a/internal/http3/roundtrip.go
+++ b/internal/http3/roundtrip.go
@@ -11,6 +11,7 @@
"strconv"
"sync"
+ "golang.org/x/net/http/httpguts"
"golang.org/x/net/internal/httpcommon"
)
@@ -113,14 +114,17 @@
return nil, err
}
+ is100ContinueReq := httpguts.HeaderValuesContainsToken(req.Header["Expect"], "100-continue")
if encr.HasBody {
- // TODO: Defer sending the request body when "Expect: 100-continue" is set.
rt.reqBody = req.Body
rt.reqBodyWriter.st = st
rt.reqBodyWriter.remain = contentLength
rt.reqBodyWriter.flush = true
rt.reqBodyWriter.name = "request"
- go copyRequestBody(rt)
+
+ if !is100ContinueReq {
+ go copyRequestBody(rt)
+ }
}
// Read the response headers.
@@ -138,7 +142,18 @@
if statusCode >= 100 && statusCode < 199 {
// TODO: Handle 1xx responses.
- continue
+ switch statusCode {
+ case 100:
+ if is100ContinueReq {
+ go copyRequestBody(rt)
+ continue
+ }
+ // If we did not send "Expect: 100-continue" request but
+ // received status 100 anyways, just continue per usual and
+ // let the caller decide what to do with the response.
+ default:
+ continue
+ }
}
// We have the response headers.
diff --git a/internal/http3/roundtrip_test.go b/internal/http3/roundtrip_test.go
index 230ff82..8a8084b 100644
--- a/internal/http3/roundtrip_test.go
+++ b/internal/http3/roundtrip_test.go
@@ -11,6 +11,7 @@
"errors"
"io"
"net/http"
+ "slices"
"testing"
"testing/synctest"
@@ -352,3 +353,71 @@
}
})
}
+
+func TestRoundTripExpect100Continue(t *testing.T) {
+ synctest.Test(t, func(t *testing.T) {
+ tc := newTestClientConn(t)
+ tc.greet()
+ bodyr, bodyw := io.Pipe()
+
+ // Client sends an Expect: 100-continue request.
+ req, _ := http.NewRequest("PUT", "https://example.tld/", bodyr)
+ req.Header = http.Header{"Expect": {"100-continue"}}
+ rt := tc.roundTrip(req)
+ st := tc.wantStream(streamTypeRequest)
+
+ // Server responds with HTTP status 100.
+ st.wantHeaders(nil)
+ st.writeHeaders(http.Header{
+ ":status": []string{"100"},
+ })
+
+ // Client sends its body after receiving HTTP status 100 response.
+ bodyw.Write(make([]byte, 1000))
+
+ // The server finishes its response after getting the client's body.
+ st.writeHeaders(http.Header{
+ ":status": []string{"200"},
+ })
+ bodyContent := []byte("success!")
+ st.writeData(bodyContent)
+ st.stream.stream.CloseWrite()
+ rt.wantStatus(200)
+ if gotBody, err := io.ReadAll(rt.response().Body); !slices.Equal(gotBody, bodyContent) || err != nil {
+ t.Errorf("Reading Response.Body returns %v, %v; want %v, %v", string(gotBody), err, string(bodyContent), nil)
+ }
+ if err := rt.response().Body.Close(); err != nil {
+ t.Errorf("Response.Body.Close() = %v, want nil", err)
+ }
+ })
+}
+
+func TestRoundTripExpect100ContinueRejected(t *testing.T) {
+ synctest.Test(t, func(t *testing.T) {
+ tc := newTestClientConn(t)
+ tc.greet()
+ bodyr, _ := io.Pipe()
+
+ // Client sends an Expect: 100-continue request.
+ req, _ := http.NewRequest("PUT", "https://example.tld/", bodyr)
+ req.Header = http.Header{"Expect": {"100-continue"}}
+ rt := tc.roundTrip(req)
+ st := tc.wantStream(streamTypeRequest)
+
+ // Server rejects it.
+ st.wantHeaders(nil)
+ st.writeHeaders(http.Header{
+ ":status": []string{"403"},
+ })
+ rejectBody := []byte("not allowed")
+ st.writeData(rejectBody)
+ st.stream.stream.CloseWrite()
+ rt.wantStatus(403)
+ if gotBody, err := io.ReadAll(rt.response().Body); !slices.Equal(gotBody, rejectBody) || err != nil {
+ t.Errorf("Reading Response.Body returns %v, %v; want %v, %v", string(gotBody), err, string(rejectBody), nil)
+ }
+ if err := rt.response().Body.Close(); err != nil {
+ t.Errorf("Response.Body.Close() = %v, want nil", err)
+ }
+ })
+}
Change information
- M internal/http3/roundtrip.go
- M internal/http3/roundtrip_test.go
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil added 7 comments![]( "Open in Gerrit")
go copyRequestBody(rt)What happens if we get multiple 100 Continue responses?
req, _ := http.NewRequest("PUT", "https://example.tld/", bodyr)We should provide the request with a body that contains data, since this test should verify that the client does not send that data until it receives a 100 Continue.
st := tc.wantStream(streamTypeRequest)We want a `tc.wantIdle()` here to verify that the client hasn't sent the body yet.
bodyw.Write(make([]byte, 1000))This should be `tc.wantData(body)` to verify that the client now sends the body.
}Checking the body should be: `rt.wantBody(bodyContent)`.
I don't think this is implemented in this package yet; take a look at x/net/http2's `testRoundTrip` in `clientconn_test.go` for the HTTP/2 equivalent.
My philosophy of testing for the various HTTP packages is that any condition we want to test for more than once or twice should be easily expressible as a single-line assertion.
":status": []string{"403"},Somewhat trivial, but let's use a 200 response here as we do in the test above. The key distinction between these test cases is whether the server sends a 100 Continue or not, not the response code sent by the server. Keeping everything other than the 100 Continue the same makes it clear what we're testing for.
})Add a `tc.wantIdle()` here to verify that the client doesn't send the body.
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin added 7 comments![]( "Open in Gerrit")
What happens if we get multiple 100 Continue responses?
Oops right. Let me use `encr.HasBody` to guard from multiple `copyRequestBody`.
req, _ := http.NewRequest("PUT", "https://example.tld/", bodyr)We should provide the request with a body that contains data, since this test should verify that the client does not send that data until it receives a 100 Continue.
Oops, indeed. Done!
We want a `tc.wantIdle()` here to verify that the client hasn't sent the body yet.
Done
This should be `tc.wantData(body)` to verify that the client now sends the body.
Done
Checking the body should be: `rt.wantBody(bodyContent)`.
I don't think this is implemented in this package yet; take a look at x/net/http2's `testRoundTrip` in `clientconn_test.go` for the HTTP/2 equivalent.
My philosophy of testing for the various HTTP packages is that any condition we want to test for more than once or twice should be easily expressible as a single-line assertion.
Sounds good, done!
Somewhat trivial, but let's use a 200 response here as we do in the test above. The key distinction between these test cases is whether the server sends a 100 Continue or not, not the response code sent by the server. Keeping everything other than the 100 Continue the same makes it clear what we're testing for.
Sure. I opted for 403 because it felt more like actual real-world scenario, which I thought would help with understanding the test. Point taken on how it can create misunderstanding on what is actually being tested though.
Add a `tc.wantIdle()` here to verify that the client doesn't send the body.
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil voted Code-Review+2![]( "Open in Gerrit")
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin submitted the change![]( "Open in Gerrit")
Change information
internal/http3: add Expect: 100-continue support to ClientConn
When sending a request containing the "Expect: 100-continue" header,
ClientConn.RoundTrip will now only send the request body after receiving
an HTTP 100 status response from the server.
For golang/go#70914
- M http2/clientconn_test.go
- M internal/http3/roundtrip.go
- M internal/http3/roundtrip_test.go
- M internal/http3/transport_test.go
Code-Review: +2 by Damien Neil, +1 by Nicholas Husin
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |