x/net/http2/h2c: h2c server hangs on empty r.Body read and fails on writes + hangs for POSTs #52882
Labels
Milestone
Comments
ldemailly
changed the title
ldemailly
changed the title
|
Forgot to mention that this is with and just rechecked, same with go1.18.2 just the error is only on 2nd write: |
|
Code is also available on https://github.com/fortio/h2-bug |
|
cc @neild |
heschi
added
the
NeedsInvestigation
|
Thanks! Note that the same happens when trying to use ReverseProxy as well (which is how I went down this path:)
Server httpURL, _ := url.Parse("http://localhost:8080/")
httpProxy := httputil.NewSingleHostReverseProxy(httpURL)
mux := http.NewServeMux()
mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
fmt.Printf("web req: %v\n", r.URL)
httpProxy.ServeHTTP(w, r)
})
h2s := &http2.Server{}
srv := &http.Server{
Addr: ":8001",
Handler: h2c.NewHandler(mux, h2s),
}
//http2.ConfigureServer(srv, nil)
log.Fatal(srv.ListenAndServe())curl side logs (failing upgrade:): prior knowledge (working): working server side: |
seankhliao
changed the title
|
I think the server is not correctly configured. Here is the revised version that works: https://go.dev/play/p/hO_OnkMS-wz I will highlight the changes at the end of this comment. According to the doc of https://pkg.go.dev/golang.org/x/net/http2#ConfigureServer,
And here is the now deleted http2 server demo that could be helpful: https://github.com/golang/net/blob/69e39bad7dc2bbb411fa35755c46020969029fa7/http2/h2demo/h2demo.go package main
import (
- "crypto/tls"
"fmt"
"io"
"log"
- "net"
"net/http"
- "net/url"
"golang.org/x/net/http2"
- "golang.org/x/net/http2/h2c"
)
func server() {
h2s := &http2.Server{}
handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
fmt.Printf("Got %v %v request from %v\n", r.Method, r.Proto, r.RemoteAddr)
body, err := io.ReadAll(r.Body)
if err != nil {
log.Fatalf("server body read err: %v\n", err)
}
fmt.Printf("read %d from body\n", len(body))
w.WriteHeader(http.StatusAccepted)
fmt.Fprintf(w, "Hello, %v; HTTP Version: %v\n\nbody:\n%s\n", r.URL.Path, r.Proto, string(body))
})
+ http.HandleFunc("/", handler)
+
server := &http.Server{
Addr: ":8001",
- Handler: h2c.NewHandler(handler, h2s),
}
+ http2.ConfigureServer(server, h2s)
fmt.Printf("H2c Server starting on %s\n", server.Addr)
err := server.ListenAndServe()
if err != nil {
panic(err)
}
}
func main() {
server()
} |
|
@ZekeLu this issue is about the h2c (http2 using cleartext) upgrade process. your example drops the important part out |
|
Ah, I totally missed the h2c part. Thank you for pointing that out and sorry for making the noise. |
|
yeah as I mentioned in the description (sorry if it's noisy/unclear) |
|
The It then takes those converted HTTP/2 bytes, prepends them to the subsequent HTTP/2 stream sent from the client, and presents this to the HTTP/2 server impementation. The conversion from HTTP/1 to HTTP/2 does not appear to support request bodies at all, so any initial HTTP/1 request with a body is mishandled. Also, I think reading from the request body at all (even when it's not present) may be broken due to the synthetic HTTP/2 request not including an END_STREAM flag. To make things even more fun, a fundamental problem with this approach is that it doesn't deal with HTTP/2 flow control at all. I honestly have no idea how to fix this without completely rearchitecting h2c support. If we want to support h2c, we should stop rewriting HTTP/1 requests into HTTP/2 and have a real mechanism for handing off an upgraded request to the HTTP/2 server. |
|
Change https://go.dev/cl/407454 mentions this issue: |
|
I used the your cl branch and I can confirm this fixes the issue I reported! thanks so much! do you know when it'll be available on the main branch? while I have you I would love an h2c upgrade client and a way for the ReverseProxy to do h2c prior knowledge (h2c:// urls?) |
|
Answering my own question, I got it working !! rp := httputil.ReverseProxy{
Director: Director,
Transport: &http2.Transport{
AllowHTTP: true,
DialTLS: func(network, addr string, cfg *tls.Config) (net.Conn, error) {
return net.Dial(network, addr)
},
},
}Still wondering how to configure as h2c upgrade though so it would work with h1.1 targets with a single transport |
ldemailly
added a commit
to fortio/proxy
that referenced
this issue
May 20, 2022
ldemailly
added a commit
to fortio/proxy
that referenced
this issue
May 20, 2022
h2c now workin Thx to fix for golang/go#52882
|
unfortunately because of #44840 (comment) I can't seem to use my fork to make a release (do I miss something obvious in my go.mod?) Edit: found a workaround, change the import and go.mod in the fork... |
ldemailly
added a commit
to fortio/proxy
that referenced
this issue
May 25, 2022
ldemailly
added a commit
to fortio/fortio
that referenced
this issue
May 25, 2022
ldemailly
added a commit
to fortio/fortio
that referenced
this issue
May 25, 2022
* use h2c fixed version of x/net ( golang/go#52882 ) * add a test using curl from the build image * add max 10s timeout for when the test fails (on master or future regressions) * prep for 1.32.1
dteh
pushed a commit
to dteh/fhttp
that referenced
this issue
Jun 22, 2022
The initial request on an h2c-upgraded connection is written as an HTTP/1 request, with the response sent as an HTTP/2 stream. The h2c package handled this request by constructing a sequence of bytes representing an HTTP/2 stream containing the initial request, prepending those bytes to the remainder of the connection, and presenting that to the HTTP/2 server as if no upgrade had happened. This translation did not handle request bodies. Handling request bodies under this model would be difficult, since it would require also translating the HTTP/2 flow control. Rewrite the h2c upgrade to explicitly hand off the request to the HTTP/2 server instead. Fixes golang/go#52882. Change-Id: I26e0f12e2b1c8b48fd36ba47baea076424983553 Reviewed-on: https://go-review.googlesource.com/c/net/+/407454 Reviewed-by: Brad Fitzpatrick <[email protected]> Run-TryBot: Damien Neil <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]>
dmitshur
added
NeedsFix
WeiminShang
added a commit
to WeiminShang/net
that referenced
this issue
Nov 16, 2022
The initial request on an h2c-upgraded connection is written as an HTTP/1 request, with the response sent as an HTTP/2 stream. The h2c package handled this request by constructing a sequence of bytes representing an HTTP/2 stream containing the initial request, prepending those bytes to the remainder of the connection, and presenting that to the HTTP/2 server as if no upgrade had happened. This translation did not handle request bodies. Handling request bodies under this model would be difficult, since it would require also translating the HTTP/2 flow control. Rewrite the h2c upgrade to explicitly hand off the request to the HTTP/2 server instead. Fixes golang/go#52882. Change-Id: I26e0f12e2b1c8b48fd36ba47baea076424983553 Reviewed-on: https://go-review.googlesource.com/c/net/+/407454 Reviewed-by: Brad Fitzpatrick <[email protected]> Run-TryBot: Damien Neil <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
h2c server - with code to read request body:
https://go.dev/play/p/yaKhF5u4ut6
What did you expect to see?
to succeed - it hangs
note that --http2-prior-knowledge or --http1.1 works fine
What did you see instead?
hang on curl side, server side:
If not doing a POST (ie without a body) the ReadAll hangs (that I could workaround by checking the method, even though it probably shouldn't hang)
The text was updated successfully, but these errors were encountered: