[net] internal/http3: add Expect: 100-continue support to Server
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 Server
When serving a request containing the "Expect: 100-continue" header,
Server will now send an HTTP 100 status automatically if the request
body is read from within the server handler.
For golang/go#70914
Change diff
diff --git a/internal/http3/body.go b/internal/http3/body.go
index fc758bd..df06432 100644
--- a/internal/http3/body.go
+++ b/internal/http3/body.go
@@ -57,6 +57,10 @@
mu sync.Mutex
remain int64
err error
+ // If not nil, the body contains an "Expect: 100-continue" header, and
+ // send100Continue should be called when Read is invoked for the first
+ // time.
+ send100Continue func()
}
func (r *bodyReader) Read(p []byte) (n int, err error) {
@@ -65,6 +69,10 @@
// Use a mutex here to provide the same behavior.
r.mu.Lock()
defer r.mu.Unlock()
+ if r.send100Continue != nil {
+ r.send100Continue()
+ r.send100Continue = nil
+ }
if r.err != nil {
return 0, r.err
}
diff --git a/internal/http3/server.go b/internal/http3/server.go
index a6976c0..2818811 100644
--- a/internal/http3/server.go
+++ b/internal/http3/server.go
@@ -220,6 +220,15 @@
}
defer rw.close()
+ has100Continue := httpguts.HeaderValuesContainsToken(req.Header["Expect"], "100-continue")
+ if has100Continue {
+ delete(req.Header, "Expect")
+ req.Body.(*bodyReader).send100Continue = func() {
+ rw.WriteHeader(http.StatusContinue)
+ rw.Flush()
+ }
+ }
+
// TODO: handle panic coming from the HTTP handler.
sc.handler.ServeHTTP(rw, req)
return nil
@@ -238,11 +247,10 @@
}
type responseWriter struct {
- st *stream
- bw *bodyWriter
- mu sync.Mutex
- headers http.Header
- // TODO: support 1xx status
+ st *stream
+ bw *bodyWriter
+ mu sync.Mutex
+ headers http.Header
wroteHeader bool // Non-1xx header has been (logically) written.
isHeadResp bool // response is for a HEAD request.
}
@@ -278,7 +286,9 @@
rw.st.writeVarint(int64(frameTypeHeaders))
rw.st.writeVarint(int64(len(encHeaders)))
rw.st.Write(encHeaders)
- rw.wroteHeader = true
+ if statusCode >= http.StatusOK {
+ rw.wroteHeader = true
+ }
}
func (rw *responseWriter) WriteHeader(statusCode int) {
diff --git a/internal/http3/server_test.go b/internal/http3/server_test.go
index b7f195d..9b34709 100644
--- a/internal/http3/server_test.go
+++ b/internal/http3/server_test.go
@@ -263,6 +263,73 @@
})
}
+func TestServerExpect100Continue(t *testing.T) {
+ synctest.Test(t, func(t *testing.T) {
+ ts := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ // Expect: 100-continue header should not be acceesible from the
+ // server handler.
+ if len(r.Header) > 0 {
+ t.Errorf("want request header to be empty, got %v", r.Header)
+ }
+ // Reading the body will cause the server to call w.WriteHeader(100).
+ body, err := io.ReadAll(r.Body)
+ if err != nil {
+ t.Fatal(err)
+ }
+ // Implicitly calls w.WriteHeader(200) since non-1XX status code
+ // has been sent yet so far.
+ w.Write(body)
+ }))
+ tc := ts.connect()
+ tc.greet()
+
+ // Client sends an Expect: 100-continue request.
+ reqStream := tc.newStream(streamTypeRequest)
+ reqStream.writeHeaders(http.Header{
+ ":method": {http.MethodGet},
+ "Expect": {"100-continue"},
+ })
+
+ // Wait until server responds with HTTP status 100 before sending the
+ // body.
+ synctest.Wait()
+ reqStream.wantHeaders(http.Header{":status": {"100"}})
+ body := []byte("body that will be echoed back if we get status 100")
+ reqStream.writeData(body)
+ reqStream.stream.stream.CloseWrite()
+
+ // Receive the server's response after sending the body.
+ reqStream.wantHeaders(http.Header{":status": {"200"}})
+ reqStream.wantData(body)
+ reqStream.wantClosed("request is complete")
+ })
+}
+
+func TestServerExpect100ContinueRejected(t *testing.T) {
+ synctest.Test(t, func(t *testing.T) {
+ rejectBody := []byte("not allowed")
+ ts := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ w.WriteHeader(403)
+ w.Write(rejectBody)
+ }))
+ tc := ts.connect()
+ tc.greet()
+
+ // Client sends an Expect: 100-continue request.
+ reqStream := tc.newStream(streamTypeRequest)
+ reqStream.writeHeaders(http.Header{
+ ":method": {http.MethodGet},
+ "Expect": {"100-continue"},
+ })
+
+ // Server rejects it.
+ synctest.Wait()
+ reqStream.wantHeaders(http.Header{":status": {"403"}})
+ reqStream.wantData(rejectBody)
+ reqStream.wantClosed("request is complete")
+ })
+}
+
type testServer struct {
t testing.TB
s *Server
Change information
- M internal/http3/body.go
- M internal/http3/server.go
- M internal/http3/server_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 5 comments![]( "Open in Gerrit")
has100Continue := httpguts.HeaderValuesContainsToken(req.Header["Expect"], "100-continue")I believe we can use x/net/internal/httpcommon.NewServerRequest here (or maybe in `parseRequest`), which will handle detection of 100-continue headers and a variety of other things which should be identical in the http/2 and http/3 cases.
delete(req.Header, "Expect")Looks like we delete this header for HTTP/2 but not HTTP/1? Weird inconsistency, we should probably fix that someday. Matching HTTP/2 behavior seems fine here.
}The HTTP/1 server sends a 417 Expectation Failed result for any unrecognized Expect header, but the HTTP/2 server seems not to. Another weird inconsistency, and again matching HTTP/2 behavior for now seems fine.
// Expect: 100-continue header should not be acceesible from thetypo: accessible
reqStream.wantHeaders(http.Header{":status": {"100"}})Let's also verify that the server does not send a 100 Continue before reading from the body.
We can do this by making the server handler block before the ReadAll and unblocking it after we check that the conn is idle. Alternatively, we could add something like `serverTester.nextHandlerCall` from x/net/http2. I think we're going to want something like nextHandlerCall in the future; it's a useful piece of infrastructure. It doesn't help all that much in this particular case, however, so up to you whether you want to take a diversion to implement it now.
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 4 comments![]( "Open in Gerrit")
has100Continue := httpguts.HeaderValuesContainsToken(req.Header["Expect"], "100-continue")I believe we can use x/net/internal/httpcommon.NewServerRequest here (or maybe in `parseRequest`), which will handle detection of 100-continue headers and a variety of other things which should be identical in the http/2 and http/3 cases.
Thanks! NewServerRequest makes this a lot easier.
delete(req.Header, "Expect")Looks like we delete this header for HTTP/2 but not HTTP/1? Weird inconsistency, we should probably fix that someday. Matching HTTP/2 behavior seems fine here.
Huh, thanks for noting that. I'll add it to my TODO to fix these inconsistencies (and refer to both HTTP/1 and HTTP/2 for future changes).
// Expect: 100-continue header should not be acceesible from theNicholas Husintypo: accessible
Oops, fixed.
Let's also verify that the server does not send a 100 Continue before reading from the body.
We can do this by making the server handler block before the ReadAll and unblocking it after we check that the conn is idle. Alternatively, we could add something like `serverTester.nextHandlerCall` from x/net/http2. I think we're going to want something like nextHandlerCall in the future; it's a useful piece of infrastructure. It doesn't help all that much in this particular case, however, so up to you whether you want to take a diversion to implement it now.
Sounds good, done!
I have `nextHandlerCall` implemented but didn't find it that useful here. I'll just stash it for now or send it as a separate change later.
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 3 comments![]( "Open in Gerrit")
func (sc *serverConn) parseHeader(st *stream) (http.Header, *pseudoHeader, error) {Escape analysis and/or inlining might figure out that the *pseudoHeader return doesn't need to be allocated, but let's make this a non-pointer return to make it clear.
t.Errorf("want request header to be empty, got %v", r.Header)Minor, but: Standard style is got, then want. (here and below)
t.Errorf("want 0.0.0.0:8080 host, got %v", r.Host)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")
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 3 comments![]( "Open in Gerrit")
func (sc *serverConn) parseHeader(st *stream) (http.Header, *pseudoHeader, error) {Escape analysis and/or inlining might figure out that the *pseudoHeader return doesn't need to be allocated, but let's make this a non-pointer return to make it clear.
Good point, done.
t.Errorf("want request header to be empty, got %v", r.Header)Minor, but: Standard style is got, then want. (here and below)
Oops, thanks. I seem to have remembered it as the other way around for a while now, ouch. Let me change it for all instances in this file for now...
want fake.tld:1234, not 0.0.0.0:8080
- 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 Server
When serving a request containing the "Expect: 100-continue" header,
Server will now send an HTTP 100 status automatically if the request
body is read from within the server handler.
For golang/go#70914
- M internal/http3/body.go
- M internal/http3/server.go
- M internal/http3/server_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. |