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.14286% with 3 lines in your changes missing coverage. Please review.

Project coverage is 82.35%. Comparing base (7505bf2) to head (e73c121).
Report is 1 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.32%   82.35%   +0.02%     
==========================================
  Files         388      389       +1     
  Lines       39016    39098      +82     
==========================================
+ Hits        32121    32199      +78     
- Misses       5574     5577       +3     
- Partials     1321     1322       +1

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

... and 22 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.

It's not a typo. :)

"Two spaces after periods" was recommended when typewriters were used, because they were fixed-width, and it helped with readability:

https://graphicdesign.stackexchange.com/questions/2160/typesetting-two-spaces-after-the-period

I prefer continuing the tradition with source code since it isn't rendered in a variable-width font (also it's a habit), but I am fine with people wanting to use one space. We shouldn't go around changing it from one way to the other, though, as it creates churn and is just a preference, not a typo.

Thanks for sharing, wasn't aware of this.

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.

internal/transport/http2_server.go

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.

internal/leakcheck/leakcheck.go

internal/transport/http2_server.go

Co-authored-by: Doug Fawley <[email protected]>

…c#8071)

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

dfawley reviewed Feb 25, 2025

View reviewed changes

internal/leakcheck/leakcheck.go Outdated Show resolved Hide resolved

internal/transport/http2_server.go Show resolved Hide resolved

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

arjan-bal and others added 2 commits February 26, 2025 18:23

Fix typo

53d85bc

Co-authored-by: Doug Fawley <[email protected]>

Merge remote-tracking branch 'source/master' into server-side-rst-stream

217a5d3

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

dfawley approved these changes Feb 27, 2025

View reviewed changes

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

Merge remote-tracking branch 'source/master' into server-side-rst-stream

e73c121

arjan-bal merged commit 0d6e39f into grpc:master Feb 28, 2025
15 checks passed

janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Mar 24, 2025

transport: Send RST stream from the server when deadline expires (grp…

7ccc573

…c#8071)

arjan-bal mentioned this pull request May 3, 2025

streaming: should return cancel error instead of Internal error with unexpected EOF #8281

Closed

This was referenced May 13, 2025

Server should send RST_STREAM when deadline is exceeded #2886

Closed

Allow server to cancel when deadline expires connectrpc/conformance#999

Merged

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

		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

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

Uh oh!

Conversation

arjan-bal commented Feb 10, 2025

Uh oh!

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

Uh oh!

Codecov Report

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Feb 10, 2025 •

edited

Loading