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

arjan-bal · 2025-08-18T10:55:38Z

The mutex that guards the trailers should be held while copying the trailers. We do lock the mutex in the regular gRPC server transport, but have missed it in the std lib http/2 transport. The only place where a write happens is writeStatus() is when the status contains a proto.

grpc-go/internal/transport/handler_server.go

Lines 251 to 252 in 4375c78

    
           if p := st.Proto(); p != nil && len(p.Details) > 0 { 
        
           	delete(s.trailer, grpcStatusDetailsBinHeader)

RELEASE NOTES:

transport: Fix a data race while copying headers for stats handlers in the std lib http2 server transport.

codecov · 2025-08-18T10:59:16Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.00%. Comparing base (0ebea3e) to head (31a6f26).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8519      +/-   ##
==========================================
+ Coverage   81.87%   82.00%   +0.12%     
==========================================
  Files         413      413              
  Lines       40518    40520       +2     
==========================================
+ Hits        33176    33229      +53     
+ Misses       5967     5908      -59     
- Partials     1375     1383       +8

Files with missing lines	Coverage Δ
internal/transport/handler_server.go	`90.81% <100.00%> (+4.35%)`	⬆️

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

arjan-bal · 2025-08-18T16:35:45Z

internal/transport/handler_server.go

@@ -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()


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.

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.

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

}

}

}

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(),

})

}

dfawley · 2025-08-18T20:46:08Z

internal/transport/handler_server.go

@@ -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()


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.

dfawley · 2025-08-18T20:50:50Z

internal/transport/handler_server_test.go

+		if err := s.SetTrailer(metadata.Pairs("custom-trailer", "Custom trailer value")); err != nil {
+			t.Error(err)
+		}


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?

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.

dfawley · 2025-08-18T20:51:55Z

internal/transport/handler_server_test.go

+	// 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())


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

…ler_server (grpc#8519) Fixes: grpc#8514 The mutex that guards the trailers should be held while copying the trailers. We do lock the mutex in [the regular gRPC server transport](https://github.com/grpc/grpc-go/blob/9ac0ec87ca2ecc66b3c0c084708aef768637aef6/internal/transport/http2_server.go#L1140-L1142), but have missed it in the std lib http/2 transport. The only place where a write happens is `writeStatus()` is when the status contains a proto. https://github.com/grpc/grpc-go/blob/4375c784450aa7e43ff15b8b2879c896d0917130/internal/transport/handler_server.go#L251-L252 RELEASE NOTES: * transport: Fix a data race while copying headers for stats handlers in the std lib http2 server transport.

Original PR: #8519 Related issue: #8514 RELEASE NOTES: * transport: Fix a data race while copying headers for stats handlers in the std lib http2 server transport.

…ler_server (grpc#8519) Fixes: grpc#8514 The mutex that guards the trailers should be held while copying the trailers. We do lock the mutex in [the regular gRPC server transport](https://github.com/grpc/grpc-go/blob/9ac0ec87ca2ecc66b3c0c084708aef768637aef6/internal/transport/http2_server.go#L1140-L1142), but have missed it in the std lib http/2 transport. The only place where a write happens is `writeStatus()` is when the status contains a proto. https://github.com/grpc/grpc-go/blob/4375c784450aa7e43ff15b8b2879c896d0917130/internal/transport/handler_server.go#L251-L252 RELEASE NOTES: * transport: Fix a data race while copying headers for stats handlers in the std lib http2 server transport.

acquire header mutex while copying trailers

2fe9a4f

arjan-bal added this to the 1.75 Release milestone Aug 18, 2025

arjan-bal added the Type: Bug label Aug 18, 2025

arjan-bal requested a review from dfawley August 18, 2025 11:00

arjan-bal assigned dfawley Aug 18, 2025

arjan-bal commented Aug 18, 2025

View reviewed changes

arjan-bal changed the title ~~transport: acquire header mutex while copying trailers in handler_server~~ transport: ensure header mutex is held while copying trailers in handler_server Aug 18, 2025

arjan-bal force-pushed the fix-trailer-race branch from 294df40 to 2fe9a4f Compare August 18, 2025 18:58

dfawley reviewed Aug 18, 2025

View reviewed changes

dfawley assigned arjan-bal and unassigned dfawley Aug 18, 2025

Use a new test

31a6f26

arjan-bal assigned dfawley and unassigned arjan-bal Aug 19, 2025

arjan-bal force-pushed the fix-trailer-race branch from 55455d7 to 31a6f26 Compare August 20, 2025 21:31

dfawley approved these changes Aug 20, 2025

View reviewed changes

dfawley assigned arjan-bal and unassigned dfawley Aug 20, 2025

arjan-bal merged commit 5ed7cf6 into grpc:master Aug 21, 2025
29 checks passed

arjan-bal deleted the fix-trailer-race branch August 21, 2025 06:50

arjan-bal mentioned this pull request Aug 21, 2025

Cherry-pick #8519 to v1.75.x #8530

Merged

arjan-bal added a commit that referenced this pull request Aug 21, 2025

Cherry-pick #8519 to v1.75.x (#8530)

369c9aa

Original PR: #8519 Related issue: #8514 RELEASE NOTES: * transport: Fix a data race while copying headers for stats handlers in the std lib http2 server transport.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

arjan-bal commented Aug 18, 2025 •

edited

Loading

Uh oh!

codecov bot commented Aug 18, 2025 •

edited

Loading

Uh oh!

arjan-bal Aug 18, 2025 •

edited

Loading

Uh oh!

dfawley Aug 18, 2025

Uh oh!

arjan-bal Aug 19, 2025

Uh oh!

arjan-bal Aug 19, 2025

Uh oh!

dfawley Aug 18, 2025

Uh oh!

dfawley Aug 18, 2025

Uh oh!

arjan-bal Aug 19, 2025

Uh oh!

dfawley Aug 18, 2025

Uh oh!

arjan-bal Aug 19, 2025

Uh oh!

Uh oh!

Uh oh!

	if p := st.Proto(); p != nil && len(p.Details) > 0 {
	delete(s.trailer, grpcStatusDetailsBinHeader)

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

	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(),
	})
	}

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

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

Uh oh!

Conversation

arjan-bal commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arjan-bal Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

arjan-bal commented Aug 18, 2025 •

edited

Loading

codecov bot commented Aug 18, 2025 •

edited

Loading

arjan-bal Aug 18, 2025 •

edited

Loading