Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: RoundTrip hangs for request with Expect: 100-continue if the server skips continue frame #45834

Open
vans239 opened this issue Apr 28, 2021 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@vans239
Copy link

vans239 commented Apr 28, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/evanslov/Library/Caches/go-build"
GOENV="/Users/evanslov/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/evanslov/go/pkg/mod"
GONOPROXY="none"
GONOSUMDB="*.apple.com"
GOOS="darwin"
GOPATH="/Users/evanslov/go"
GOPRIVATE="*.apple.com"
GOPROXY="athens.apple.com"
GOROOT="/Users/evanslov/go/go1.16.2"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/evanslov/go/go1.16.2/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16.2"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/evanslov/work/trieste-go-services2/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/cm/gtxw2dzx26s7b7jjn0w170kh0000gn/T/go-build3714168584=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

The algorithm below is a variation of tunnelling using request&response bodies #13444

What happens:

  • client sends request with Expect: 100-continue header
  • server immediately replies with 200 OK (Continue header is not sent by server, because server hasn't read from the request, there is no way to manually trigger Continue frame from the server)
  • client gets response and tries to write to body
func TestSuccessRequest(t *testing.T) {
	var h http.HandlerFunc = func(w http.ResponseWriter, request *http.Request) {
		w.WriteHeader(http.StatusOK)
		w.(http.Flusher).Flush()

		b, err := ioutil.ReadAll(request.Body)
		if err != nil {
			log.Fatal(err)
		}
		println("Server: read " + string(b))
		w.Write([]byte("Pong"))
		w.(http.Flusher).Flush()
		println("Server: wrote response")
	}

	srv := httptest.NewUnstartedServer(h)
	srv.TLS = &tls.Config{
		NextProtos: []string{"h2"},
	}
	srv.StartTLS()

	transport := &http.Transport{
		ForceAttemptHTTP2:     true,
		ExpectContinueTimeout: time.Minute,
		TLSClientConfig: &tls.Config{
			InsecureSkipVerify: true,
		},
	}

	readerPipe, writerPipe := io.Pipe()
	ctx := httptrace.WithClientTrace(context.Background(), printlnClientTrace)
	req, err := http.NewRequestWithContext(ctx, http.MethodPost, srv.URL, readerPipe)
	if err != nil {
		log.Fatal(err)
	}
	req.Header.Set("Expect", "100-continue")
	res, err := transport.RoundTrip(req)
	if err != nil {
		log.Fatal(err)
	}
	println(res.Status)
	writerPipe.Write([]byte("ping"))
	writerPipe.Close()
	b, err := ioutil.ReadAll(res.Body)
	if err != nil {
		log.Fatal(err)
	}
	println(string(b))
}

What did you expect to see?

The client successfully writes request body and test completes

What did you see instead?

Nothing progresses until ExpectContinueTimeout is triggered (1 minute)

Thoughts

The bodyWriter doesn't read request body, because it still waits for frame 100 Continue https://github.com/golang/net/blob/master/http2/transport.go#L2022
I feel that successful response can be considered as an alternative to 100 Continue and be used to start body write.

So the fix could be:
https://github.com/golang/net/blob/89ef3d95e781148a0951956029c92a211477f7f9/http2/transport.go#L1159

handleReadLoopResponse := func(re resAndError) (*http.Response, bool, error) {
	res := re.res
	if re.err != nil || res.StatusCode > 299 {
		// On error or status code 3xx, 4xx, 5xx, etc abort any
		// ongoing write, assuming that the server doesn't care
		// about our request body. If the server replied with 1xx or
		// 2xx, however, then assume the server DOES potentially
		// want our body (e.g. full-duplex streaming:
		// golang.org/issue/13444). If it turns out the server
		// doesn't, they'll RST_STREAM us soon enough. This is a
		// heuristic to avoid adding knobs to Transport. Hopefully
		// we can keep it.
		bodyWriter.cancel()
		cs.abortRequestBodyWrite(errStopReqBodyWrite)
		if hasBody && !bodyWritten {
			<-bodyWriter.resc
		}
	}
	if re.err != nil {
		cc.forgetStreamID(cs.ID)
		return nil, cs.getStartedWrite(), re.err
	}
        // ### Changes start
	if cs.on100 != nil {
		cs.on100()
	}
        // ### Changes end
	res.Request = req
	res.TLS = cc.tlsState
	return res, false, nil
}
@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 29, 2021
@cherrymui cherrymui added this to the Backlog milestone Apr 29, 2021
@cherrymui
Copy link
Member

cc @bradfitz @neild

@networkimprov
Copy link

cc @empijei @fraenkel

@fraenkel
Copy link
Contributor

Is this an issue with http and/or http2?

@vans239
Copy link
Author

vans239 commented May 18, 2021

The issue appears with http2. I haven't been able to do experiments with http1 (other things go wrong)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants