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: undeclared HTTP/2 trailers not sent when response body is empty and unflushed #54723

Open
akshayjshah opened this issue Aug 29, 2022 · 10 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@akshayjshah
Copy link
Contributor

akshayjshah commented Aug 29, 2022

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

$ go version
go version go1.19 darwin/arm64

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=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/ashah/Library/Caches/go-build"
GOENV="/Users/ashah/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/ashah/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/ashah"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/ashah/sdk/go1.19"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/ashah/sdk/go1.19/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.19"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/ashah/src/github.com/akshayjshah/trailererr/go.mod"
GOWORK=""
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 arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/0h/bt5cl84j5zs2t3sbyyy8vgzw0000gn/T/go-build2678483231=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

In a net/http handler, I wrote some HTTP trailers to the response writer using http.TrailerPrefix. The handler never writes to or flushes the response body. (This is admittedly odd, but trailers without a body are necessary in the gRPC HTTP/2 protocol.)

What did you expect to see?

I expected the client to receive the trailers regardless of the HTTP protocol version in use.

What did you see instead?

Everything works as expected when the server and client use HTTP/1.1. Regardless of the HTTP protocol version, everything also works as expected if I stop using http.TrailerPrefix and instead set the "Trailer" header.

When the client negotiates up to HTTP/2 and the handler uses http.TrailerPrefix, the client doesn't receive any trailers unless the handler flushes the response body. This is a little strange - it doesn't match the HTTP/1.x behavior, and it's not obvious why flushing a response without any buffered data should do anything at all. (I assume that something in x/net/http2 doesn't send trailers unless we've sent a DATA frame, and perhaps flushing always sends a frame? I haven't dug far enough to verify that hypothesis.)

Here's a reproduction:

package main

import (
	"io"
	"net/http"
	"net/http/httptest"
	"testing"
)

func testTrailers(tb testing.TB, flush bool) {
	const trailerKey = "Foo"
	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		w.Header().Set(http.TrailerPrefix+trailerKey, "Bar")
		if flusher, ok := w.(http.Flusher); ok && flush {
			flusher.Flush()
		}
		if _, err := io.Copy(io.Discard, r.Body); err != nil {
			tb.Errorf("discard request body: %v", err)
		}
		if err := r.Body.Close(); err != nil {
			tb.Errorf("close request body: %v", err)
		}
	})

	srv := httptest.NewUnstartedServer(handler)
	// Replacing the next two lines with srv.Start() allows both tests to pass.
	srv.EnableHTTP2 = true
	srv.StartTLS()
	defer srv.Close()

	res, err := srv.Client().Get(srv.URL)
	if err != nil {
		tb.Fatal(err)
	}
	tb.Logf("response protocol is %s", res.Proto)
	if _, err := io.Copy(io.Discard, res.Body); err != nil {
		tb.Fatalf("discard response body: %v", err)
	}
	if res.Trailer.Get(trailerKey) == "" {
		tb.Fatalf("missing trailer %q, got %v", trailerKey, res.Trailer)
	}
}

func TestTrailersFlush(t *testing.T) {
	testTrailers(t, true /* flush */)
}

func TestTrailersWithoutFlush(t *testing.T) {
	testTrailers(t, false /* flush */)
}
@akshayjshah akshayjshah changed the title net/http: behavior of http.TrailerPrefix depends on HTTP version in use net/http: http.TrailerPrefix behavior depends on HTTP version Aug 29, 2022
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 29, 2022
@seankhliao
Copy link
Member

seankhliao commented Aug 29, 2022

cc @neild

@gopherbot
Copy link

gopherbot commented Aug 30, 2022

Change https://go.dev/cl/426157 mentions this issue: http2: [wip] http.TrailerPrefix behavior depends on HTTP version

@ianwoolf
Copy link
Contributor

ianwoolf commented Aug 30, 2022

i send a wip cl. i found rws.conn.writeHeaders return http2errStreamClosed (https://go.googlesource.com/go/+/refs/tags/go1.19/src/net/http/h2_bundle.go#6218), I am still researching. hope some help

@akshayjshah
Copy link
Contributor Author

akshayjshah commented Aug 30, 2022

@ianwoolf Any changes to the code in h2_bundle.go would need to be made in golang.org/x/net/http2. They'll be vendored into net/http during the next release cycle.

@ianwoolf
Copy link
Contributor

ianwoolf commented Aug 30, 2022

@ianwoolf Any changes to the code in h2_bundle.go would need to be made in golang.org/x/net/http2. They'll be vendored into net/http during the next release cycle.

sorry i missed that, i am sending a new cl to golang.org/x/net/http2

@gopherbot
Copy link

gopherbot commented Aug 30, 2022

Change https://go.dev/cl/426754 mentions this issue: http2: [wip] add Trailer for writeChunk

@gopherbot
Copy link

gopherbot commented Aug 30, 2022

Change https://go.dev/cl/426874 mentions this issue: http2: send undeclared trailers when body is not written

@neild
Copy link
Contributor

neild commented Aug 30, 2022

The problem is that we're checking for undeclared trailers (with the magic Trailer: prefix) only after sending the response headers, which results in the headers being sent with an END_STREAM flag to finish the request. Flushing the ResponseWriter avoids the problem by sending the headers while the handler is still running.

You can probably also work around the problem by calling WriteHeader.

https://go.dev/cl/426874 contains a fix.

@neild neild changed the title net/http: http.TrailerPrefix behavior depends on HTTP version net/http: undeclared HTTP/2 trailers not sent when response body is empty and unflushed Aug 30, 2022
@ianwoolf
Copy link
Contributor

ianwoolf commented Aug 31, 2022

The problem is that we're checking for undeclared trailers (with the magic Trailer: prefix) only after sending the response headers, which results in the headers being sent with an END_STREAM flag to finish the request. Flushing the ResponseWriter avoids the problem by sending the headers while the handler is still running.

You can probably also work around the problem by calling WriteHeader.

https://go.dev/cl/426874 contains a fix.

hmm.. i get it, thank you

@seankhliao seankhliao added this to the Go1.20 milestone Sep 3, 2022
@akshayjshah
Copy link
Contributor Author

akshayjshah commented Sep 6, 2022

Thanks for the fix, @neild! The bit of code I was looking at can predeclare trailers, so I just set the Trailers header. It's nice to have this bit of surprising behavior polished away, though :)

gopherbot pushed a commit to golang/net that referenced this issue Sep 20, 2022
Fix a bug wherein undeclared trailers (set with the "Trailer:" prefix)
were not sent when the response body is neither written nor flushed.

We were testing responseWriterState.hasTrailers before promoting
undeclared trailers into rws.trailers, resulting in the response headers
being sent with an END_STREAM flag. Promote undeclared headers earlier
so that we leave the stream open for them to be sent.

For golang/go#54723

Change-Id: Ic036925f4a7ec775282b6e474aa72249d6418b23
Reviewed-on: https://go-review.googlesource.com/c/net/+/426874
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
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

5 participants