Skip to content

transport: ensure header mutex is held while copying trailers in handler_server #8519

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Aug 21, 2025
Merged

transport: ensure header mutex is held while copying trailers in handler_server #8519

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2fe9a4f
acquire header mutex while copying trailers
arjan-bal Aug 18, 2025
31a6f26
Use a new test
arjan-bal Aug 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions internal/transport/handler_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,11 +277,13 @@ func (ht *serverHandlerTransport) writeStatus(s *ServerStream, st *status.Status
if err == nil { // transport has not been closed
// Note: The trailer fields are compressed with hpack after this call returns.
// No WireLength field is set here.
s.hdrMu.Lock()
Copy link
Contributor Author

@arjan-bal arjan-bal Aug 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're holding two locks here, this and ht.writeStatusMu (acquired at line 229). ht.writeStatusMu is only referenced in this method, so there shouldn't be a chance of deadlocks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that hdrMu is also already taken on 249, although, I'm not sure if that closure is run in the current goroutine or another one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The callback is executed in an event loop in a separate goroutine:

grpc-go/internal/transport/handler_server.go

Lines 465 to 474 in 9ac0ec8

func (ht *serverHandlerTransport) runStream() {
for {
select {
case fn := <-ht.writes:
fn()
case <-ht.closedCh:
return
}
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about calling the stats handler in the callback above, but I noticed that the http2_server transport also schedules the network write in the background and calls the stats handlers.

grpc-go/internal/transport/http2_server.go

Lines 1136 to 1143 in 9ac0ec8

t.finishStream(s, rst, http2.ErrCodeNo, trailingHeader, true)
for _, sh := range t.stats {
// Note: The trailer fields are compressed with hpack after this call returns.
// No WireLength field is set here.
sh.HandleRPC(s.Context(), &stats.OutTrailer{
Trailer: s.trailer.Copy(),
})
}

for _, sh := range ht.stats {
sh.HandleRPC(s.Context(), &stats.OutTrailer{
Trailer: s.trailer.Copy(),
})
}
s.hdrMu.Unlock()
}
ht.Close(errors.New("finished writing status"))
return err
Expand Down
31 changes: 30 additions & 1 deletion internal/transport/handler_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
"google.golang.org/grpc/codes"
"google.golang.org/grpc/mem"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/stats"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/protoadapt"
Expand Down Expand Up @@ -246,6 +247,22 @@ type handleStreamTest struct {
ht *serverHandlerTransport
}

type mockStatsHandler struct{}

func (h *mockStatsHandler) TagRPC(ctx context.Context, _ *stats.RPCTagInfo) context.Context {
return ctx
}

func (h *mockStatsHandler) HandleRPC(context.Context, stats.RPCStats) {
}

func (h *mockStatsHandler) TagConn(ctx context.Context, _ *stats.ConnTagInfo) context.Context {
return ctx
}

func (h *mockStatsHandler) HandleConn(context.Context, stats.ConnStats) {
}

func newHandleStreamTest(t *testing.T) *handleStreamTest {
bodyr, bodyw := io.Pipe()
req := &http.Request{
Expand All @@ -260,7 +277,12 @@ func newHandleStreamTest(t *testing.T) *handleStreamTest {
Body: bodyr,
}
rw := newTestHandlerResponseWriter().(testHandlerResponseWriter)
ht, err := NewServerHandlerTransport(rw, req, nil, mem.DefaultBufferPool())
// Add mock stats handlers to exercise the stats handler code path.
statsHandlers := make([]stats.Handler, 0, 5)
for range 5 {
statsHandlers = append(statsHandlers, &mockStatsHandler{})
}
ht, err := NewServerHandlerTransport(rw, req, statsHandlers, mem.DefaultBufferPool())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we parameterize this, or find some other way to cause this configuration, instead of doing it for all existing tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -485,6 +507,12 @@ func (s) TestHandlerTransport_HandleStreams_ErrDetails(t *testing.T) {

hst := newHandleStreamTest(t)
handleStream := func(s *ServerStream) {
if err := s.SendHeader(metadata.New(map[string]string{})); err != nil {
t.Error(err)
}
if err := s.SetTrailer(metadata.Pairs("custom-trailer", "Custom trailer value")); err != nil {
t.Error(err)
}
Comment on lines +563 to +565
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How hard would it be to test this in a new test instead of in an existing test that's intended for testing error details?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to use a new test. My thought process was that modifying existing tests would give us better coverage for interactions b/w different features.

s.WriteStatus(st)
}
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
Expand All @@ -501,6 +529,7 @@ func (s) TestHandlerTransport_HandleStreams_ErrDetails(t *testing.T) {
"Grpc-Status": {fmt.Sprint(uint32(statusCode))},
"Grpc-Message": {encodeGrpcMessage(msg)},
"Grpc-Status-Details-Bin": {encodeBinHeader(stBytes)},
"Custom-Trailer": []string{"Custom trailer value"},
}

checkHeaderAndTrailer(t, hst.rw, wantHeader, wantTrailer)
Expand Down