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

Comments

@vans239
Copy link

@vans239 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 this to the Backlog milestone Apr 29, 2021
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 29, 2021

Loading

@networkimprov
Copy link

@networkimprov networkimprov commented May 16, 2021

Loading

@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented May 16, 2021

Is this an issue with http and/or http2?

Loading

@vans239
Copy link
Author

@vans239 vans239 commented May 18, 2021

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants