transport: Send RST stream from the server when deadline expires #8071

arjan-bal · 2025-02-10T04:41:16Z

This PR makes the server cancel an RPC when it sees the deadline expiring instead of waiting forn the client to cancel the stream.

RELEASE NOTES:

transport: Servers send an HTTP/2 RST_STREAM frame to cancel a stream when the deadline expires.

codecov · 2025-02-10T04:47:01Z

Codecov Report

Attention: Patch coverage is 97.05882% with 3 lines in your changes missing coverage. Please review.

Project coverage is 82.49%. Comparing base (cbb5c2f) to head (a45d43b).
Report is 14 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/leakcheck/leakcheck.go	96.20%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8071      +/-   ##
==========================================
+ Coverage   82.36%   82.49%   +0.13%     
==========================================
  Files         387      388       +1     
  Lines       39065    39049      -16     
==========================================
+ Hits        32174    32214      +40     
+ Misses       5562     5538      -24     
+ Partials     1329     1297      -32

Files with missing lines	Coverage Δ
internal/grpctest/grpctest.go	`78.43% <100.00%> (+1.83%)`	⬆️
internal/internal.go	`100.00% <ø> (ø)`
internal/transport/client_stream.go	`100.00% <ø> (ø)`
internal/transport/http2_server.go	`91.63% <100.00%> (+0.40%)`	⬆️
internal/transport/server_stream.go	`95.31% <ø> (-4.69%)`	⬇️
internal/leakcheck/leakcheck.go	`89.34% <96.20%> (+4.55%)`	⬆️

... and 37 files with indirect coverage changes

dfawley · 2025-02-14T21:34:27Z

internal/grpctest/grpctest.go

 }

 // Teardown performs a leak check.
 func (Tester) Teardown(t *testing.T) {
 	leakcheck.CheckTrackingBufferPool()
+	leakcheck.CheckTimers(5 * time.Second)


This is additive with the 10s for checking goroutines. It might be nice to pass a context (or timer) to both of these instead, so they can share the 10s timeout?

Nice idea, changed.

dfawley · 2025-02-14T21:36:09Z

internal/leakcheck/leakcheck.go

+
+	originalTimeAfterFunc func(time.Duration, func()) internal.Timer
+	mu                    sync.Mutex
+	bufferCount           int


Oops, removed.

dfawley · 2025-02-14T21:37:30Z

internal/leakcheck/leakcheck.go

+type timerFactory struct {
+	logger Logger
+
+	originalTimeAfterFunc func(time.Duration, func()) internal.Timer


Is there any need for this? As opposed to just directly calling time.AfterFunc

I was thinking about a future scenarios when subtests and outer tests both start leak checks. Having the previously set function would allow wrapping the outer tests function. The global globalTimerTracker would still prevent this from working.

Removed this field. This use case can be addressed if/when it arises.

dfawley · 2025-02-14T21:42:07Z

internal/leakcheck/leakcheck.go

+// not all timers were cancelled or executed. It is invalid to invoke this
+// function without previously having invoked SetTimerTracker.
+func CheckTimers(timeout time.Duration) {
+	// Reset the internal function.


This happens at the end?

Yes, moved the comment to the end.

dfawley · 2025-02-14T21:44:01Z

internal/transport/http2_client.go

-			// of this cancellation.  Alter the status code accordingly.
+			// of this cancellation. Alter the status code accordingly.


Please revert.

It seemed like a harmless typo fix, but I’ve reverted it as requested.

dfawley · 2025-02-14T21:45:12Z

internal/leakcheck/leakcheck.go

+// SetTimerTracker replaces internal.TimerAfterFunc  with a one that tracks
+// timer creations. CheckTimers should then be invoked at the end of the test to
+// validate that all timers created have either executed or are cancelled.
+func SetTimerTracker(logger Logger) {


Can you add a test for this functionality?

Didn't notice that there were tests for this file. Added a test now.

dfawley · 2025-02-14T21:46:11Z

internal/transport/http2_server.go

+		s.deadlineTimerCancel = func() {
+			timer.Stop()
+		}


Suggested change

s.deadlineTimerCancel = func() {

timer.Stop()

}

s.deadlineTimerCancel = timer.Stop

Changed to use the cancel field on ServerStream. cancel is always set to a non-nil value, so this issue is solved.

dfawley · 2025-02-14T21:46:57Z

internal/transport/http2_server.go

+		if s.deadlineTimerCancel != nil {
+			s.deadlineTimerCancel()
+		}


Should we make sure this is always a valid function so we can just call it without the nil check?

Changed to use the existing cancel field.

dfawley · 2025-02-14T21:48:37Z

internal/transport/http2_server.go

+	oldState := s.swapState(streamDone)
+	if oldState == streamDone {


Was this a bug before? Or did we have something that prevented it? Or was the code tolerant of multiple calls?

Going through the references, it seems like closeStream can be called concurrently. The race seems rare because the call to deleteStream below removes the stream from the activeStreams map of the server. Most code paths try to get the stream from activeStreams, so they can't call closeStream anymore.

When the function does execute multiple times, the only symptoms seem to be overcounting of channelz metrics here

grpc-go/internal/transport/http2_server.go

Lines 1287 to 1293 in 8528f43

if channelz.IsOn() {

if eosReceived {

t.channelz.SocketMetrics.StreamsSucceeded.Add(1)

} else {

t.channelz.SocketMetrics.StreamsFailed.Add(1)

}

}

and possibly sending multiple RST streams to the peer because of enqueuing a cleanupStream event in the controlBuf.

dfawley · 2025-02-14T21:49:16Z

internal/transport/server_stream.go

+	cancel              context.CancelFunc // invoked at the end of stream to cancel ctx.
+	deadlineTimerCancel func()             // Invoked at the end of stream.


Do we always call cancel? Can we piggyback the timer cancellation with it if so?

Yes we do, changed to use cancel.

arjan-bal added the Type: Behavior Change Behavior changes not categorized as bugs label Feb 10, 2025

arjan-bal added this to the 1.71 Release milestone Feb 10, 2025

arjan-bal force-pushed the server-side-rst-stream branch from 805939c to 6dda22e Compare February 10, 2025 04:55

Send server side RST stream on deadline execeeded

1f1acc4

arjan-bal force-pushed the server-side-rst-stream branch 2 times, most recently from 806c418 to fca2350 Compare February 11, 2025 11:21

Fix flaking test

a97949e

arjan-bal force-pushed the server-side-rst-stream branch from fca2350 to a97949e Compare February 11, 2025 11:26

arjan-bal requested a review from dfawley February 11, 2025 11:41

arjan-bal assigned dfawley Feb 11, 2025

Add leak check for timers

be3fcc6

dfawley reviewed Feb 14, 2025

View reviewed changes

dfawley assigned arjan-bal and unassigned dfawley Feb 14, 2025

arjan-bal added 2 commits February 19, 2025 01:42

Address review comments

6c9f65f

Fix vet

a45d43b

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

arjan-bal modified the milestones: 1.71 Release, 1.72 Release Feb 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transport: Send RST stream from the server when deadline expires #8071

transport: Send RST stream from the server when deadline expires #8071

arjan-bal commented Feb 10, 2025

codecov bot commented Feb 10, 2025 •

edited

Loading

dfawley Feb 14, 2025

arjan-bal Feb 18, 2025

dfawley Feb 14, 2025

arjan-bal Feb 18, 2025

dfawley Feb 14, 2025

arjan-bal Feb 18, 2025

dfawley Feb 14, 2025

arjan-bal Feb 18, 2025

dfawley Feb 14, 2025

arjan-bal Feb 18, 2025

dfawley Feb 14, 2025

arjan-bal Feb 18, 2025

dfawley Feb 14, 2025

arjan-bal Feb 18, 2025

dfawley Feb 14, 2025

arjan-bal Feb 18, 2025

dfawley Feb 14, 2025

arjan-bal Feb 18, 2025

dfawley Feb 14, 2025

arjan-bal Feb 18, 2025

		// of this cancellation. Alter the status code accordingly.
		// of this cancellation. Alter the status code accordingly.

		oldState := s.swapState(streamDone)
		if oldState == streamDone {

	if channelz.IsOn() {
	if eosReceived {
	t.channelz.SocketMetrics.StreamsSucceeded.Add(1)
	} else {
	t.channelz.SocketMetrics.StreamsFailed.Add(1)
	}
	}

		cancel context.CancelFunc // invoked at the end of stream to cancel ctx.
		deadlineTimerCancel func() // Invoked at the end of stream.

transport: Send RST stream from the server when deadline expires #8071

Are you sure you want to change the base?

transport: Send RST stream from the server when deadline expires #8071

Conversation

arjan-bal commented Feb 10, 2025

codecov bot commented Feb 10, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 10, 2025 •

edited

Loading