examples/features/gracefulstop: add example to demonstrate server graceful stop #7865

purnesh42H · 2024-11-22T06:16:41Z

Fixes: #7843

RELEASE NOTES: None

…ceful stop

codecov · 2024-11-22T06:22:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.99%. Comparing base (87f0254) to head (0b880da).
Report is 22 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7865      +/-   ##
==========================================
+ Coverage   81.74%   81.99%   +0.25%     
==========================================
  Files         375      377       +2     
  Lines       37980    38179     +199     
==========================================
+ Hits        31045    31306     +261     
+ Misses       5622     5565      -57     
+ Partials     1313     1308       -5

see 40 files with indirect coverage changes

bkane-msft · 2024-11-22T16:56:52Z

This looks great, thank you! Would you ok linking to this example from https://grpc.io/docs/languages/go/basics/#starting-the-server so folks following the "Basics" tutorial can easily find it?

dfawley · 2024-11-22T18:25:22Z

examples/features/gracefulstop/README.md

+# Graceful Stop
+
+This example demonstrates how to gracefully stop a gRPC server using
+Server.GracefulStop().


Suggested change

Server.GracefulStop().

`Server.GracefulStop()`.

GitHub has a somewhat hidden feature that lets you offer suggestions by editing the text selected in the comment box . Then the PR author can click "accept" right there in the comment thread instead of checking out the repo and editing it.

https://haacked.com/archive/2019/06/03/suggested-changes/

Added backticks. Thanks

dfawley · 2024-11-22T18:25:56Z

examples/features/gracefulstop/README.md

+## How to run
+
+Start the server which will listen to incoming gRPC requests as well as OS
+interrupt signals (SIGINT/SIGTERM). After receiving interrupt signal, it calls


Grammar: an interrupt signal

dfawley · 2024-11-22T18:26:13Z

examples/features/gracefulstop/README.md

+Start the server which will listen to incoming gRPC requests as well as OS
+interrupt signals (SIGINT/SIGTERM). After receiving interrupt signal, it calls
+`s.GracefulStop()` to gracefully shut down the server. If graceful shutdown
+doesn't happen in time, server is stopped forcefully.


Grammar: the server

dfawley · 2024-11-22T18:27:28Z

examples/features/gracefulstop/README.md

+Use Ctrl+C or SIGTERM to signal the server to shut down.
+
+The server begins a graceful stop:
+- It finishes ongoing requests.


Nit: ongoing -> in flight / in progress

("Ongoing" means more like "continuous".)

Re-wrote the description as the logic is changed now. Using in-flight though.

dfawley · 2024-11-22T18:29:19Z

examples/features/gracefulstop/README.md

+
+The server begins a graceful stop:
+- It finishes ongoing requests.
+- Rejects new incoming requests.


I'm not sure how precise we want to be here, but this isn't exactly what happens, FYI.

The server sends a GOAWAY on the connection, which the client sees, and then the client will stop sending requests on the connection. If the client ignores the GOAWAY, then the server will reject new streams, but that shouldn't actually happen in any properly functioning HTTP/2 system.

What you wrote might be good enough for a basics explanation though.

Thanks. Now i am just restricting the description to completing the in-flight requests because we are not demonstrating sending new request while server is shutting down.

examples/features/gracefulstop/client/main.go

dfawley · 2024-11-22T18:32:21Z

examples/features/gracefulstop/client/main.go

+	if err != nil {
+		log.Fatalf("Failed to connect: %v", err)
+	}


Nit: NewClient doesn't connect. The only errors you'd get here would be providing illegal options to the client.

dfawley · 2024-11-22T18:33:15Z

examples/features/gracefulstop/client/main.go

+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	for i := 1; i <= 10; i++ {


Shouldn't the client do these until they fail with Unavailable?

yes, client now waits until receiving an error from server. Explained above.

dfawley · 2024-11-22T18:38:09Z

examples/features/gracefulstop/server/main.go

+		<-stop
+		fmt.Println("Shutting down server...")
+		s.GracefulStop()
+		close(stop)


Is this OK? Closing the channel given to signal.Notify? What if ^C is pressed again after the channel is closed? Wouldn't it panic?

Better to just do a timer := time.AfterFunc(10*time.Second, s.Stop); defer timer.Stop(); s.GracefulStop() here?

purnesh42H · 2024-11-25T06:09:01Z

This looks great, thank you! Would you ok linking to this example from https://grpc.io/docs/languages/go/basics/#starting-the-server so folks following the "Basics" tutorial can easily find it?

@bkane-msft the basic tutorial might not be appropriate for a graceful stop example as it is slightly advanced in nature. Right now we are proposing to add it as a cross-language "guide" https://grpc.io/docs/guides/. Will update here.

dfawley · 2024-11-25T18:33:04Z

examples/features/gracefulstop/server/main.go

+			if streamMessages > 5 {
+				return fmt.Errorf("request failed")
+			}


Why return an error? Exiting successfully feels more graceful.

Also you should always return status errors from your method handlers. This will get converted to UNKNOWN by gRPC, which "is fine", but we should never show an example that does anything but send statuses back.

Not applicable now. Explained the flow above

dfawley · 2024-11-25T18:37:14Z

examples/features/gracefulstop/client/main.go

+		if err != nil {
+			// Handle the error and close the stream gracefully
+			log.Printf("Error sending request: %v\n", err)
+			err := stream.CloseSend()


This doesn't make sense. The stream ended (by virtue of receiving an error), so there is no reason to CloseSend. Probably CloseSend should also return an error in this scenario, but it's also probably not important.

Not applicable now

examples/features/gracefulstop/client/main.go

bkane-msft · 2024-11-26T17:00:41Z

@purnesh42H and @dfawley , as someone new to gRPC, I'm really grateful to you two for your hard work in this PR providing a correct production-ready shutdown example! Thank you!

dfawley

The example LGTM. Please add coverage for it in examples_test.sh

purnesh42H · 2024-12-03T04:19:52Z

The example LGTM. Please add coverage for it in examples_test.sh

Added

dfawley · 2024-12-05T16:56:28Z

examples/examples_test.sh

@@ -71,6 +71,7 @@ EXAMPLES=(
    "features/orca"
    "features/retry"
    "features/unix_abstract"
+    "features/gracefulstop"


This needs expectations for server and/or client output to confirm it is working correctly.

Added. PTAL

dfawley · 2024-12-05T22:43:35Z

LGTM. Should the PR comment 0 say "Fixes #7843" instead so it's closed?

purnesh42H · 2024-12-06T05:00:36Z

LGTM. Should the PR comment 0 say "Fixes #7843" instead so it's closed?

yeah we can close this issue but I think we should link it through grpc.io docs with similar guidelines for server graceful stop.

…ceful stop (grpc#7865)

examples/features/gracefulstop: add example to demonstrate server gra…

6b8ce78

…ceful stop

purnesh42H force-pushed the example-gracefulstop branch from c120ffd to 6b8ce78 Compare November 22, 2024 06:17

purnesh42H requested review from dfawley and arjan-bal November 22, 2024 06:17

purnesh42H assigned dfawley and arjan-bal Nov 22, 2024

purnesh42H added the Type: Documentation Documentation or examples label Nov 22, 2024

purnesh42H added this to the 1.69 Release milestone Nov 22, 2024

dfawley reviewed Nov 22, 2024

View reviewed changes

dfawley assigned purnesh42H and unassigned dfawley and arjan-bal Nov 22, 2024

use ServerStreaming scenario to demonstrate graceful shutdown

5ee5e3b

purnesh42H force-pushed the example-gracefulstop branch 4 times, most recently from 34c46e8 to 96ebe95 Compare November 25, 2024 05:57

purnesh42H requested a review from dfawley November 25, 2024 06:15

purnesh42H assigned dfawley and unassigned purnesh42H Nov 25, 2024

use ServerStreaming scenario to demonstrate graceful shutdown

d48bd1f

purnesh42H force-pushed the example-gracefulstop branch from 96ebe95 to d48bd1f Compare November 25, 2024 06:38

dfawley reviewed Nov 25, 2024

View reviewed changes

dfawley assigned purnesh42H and unassigned dfawley Nov 25, 2024

purnesh42H requested a review from dfawley November 27, 2024 13:36

purnesh42H added 2 commits November 28, 2024 22:31

address structure comments

f4424d0

address structure comments

2ddb80e

purnesh42H requested a review from dfawley November 28, 2024 17:08

purnesh42H assigned dfawley and unassigned purnesh42H Nov 28, 2024

dfawley reviewed Dec 2, 2024

View reviewed changes

dfawley assigned purnesh42H and unassigned dfawley Dec 2, 2024

add features/gracefulstop to example_test.sh

c8e4d3a

purnesh42H requested a review from dfawley December 3, 2024 04:19

purnesh42H assigned dfawley and unassigned purnesh42H Dec 3, 2024

purnesh42H modified the milestones: 1.69 Release, 1.70 Release Dec 5, 2024

dfawley reviewed Dec 5, 2024

View reviewed changes

dfawley assigned purnesh42H and unassigned dfawley Dec 5, 2024

add client and server expectation outputs

0b880da

purnesh42H force-pushed the example-gracefulstop branch from 91c01e0 to 0b880da Compare December 5, 2024 17:12

purnesh42H requested a review from dfawley December 5, 2024 17:18

purnesh42H assigned dfawley and unassigned purnesh42H Dec 5, 2024

dfawley approved these changes Dec 5, 2024

View reviewed changes

dfawley assigned purnesh42H and unassigned dfawley Dec 5, 2024

purnesh42H merged commit 66ba4b2 into grpc:master Dec 6, 2024
15 checks passed

purnesh42H added a commit to purnesh42H/grpc-go that referenced this pull request Dec 18, 2024

examples/features/gracefulstop: add example to demonstrate server gra…

41a1c74

…ceful stop (grpc#7865)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

examples/features/gracefulstop: add example to demonstrate server graceful stop #7865

examples/features/gracefulstop: add example to demonstrate server graceful stop #7865

purnesh42H commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

bkane-msft commented Nov 22, 2024

dfawley Nov 22, 2024

bkane-msft Nov 22, 2024

bkane-msft Nov 22, 2024 •

edited

Loading

bkane-msft Nov 22, 2024

purnesh42H Nov 25, 2024

dfawley Nov 22, 2024

purnesh42H Nov 25, 2024

dfawley Nov 22, 2024

purnesh42H Nov 25, 2024

dfawley Nov 22, 2024

purnesh42H Nov 25, 2024

dfawley Nov 22, 2024

purnesh42H Nov 25, 2024

dfawley Nov 22, 2024

purnesh42H Nov 25, 2024

dfawley Nov 22, 2024

purnesh42H Nov 25, 2024

dfawley Nov 22, 2024

purnesh42H Nov 25, 2024

purnesh42H commented Nov 25, 2024 •

edited

Loading

dfawley Nov 25, 2024

purnesh42H Nov 27, 2024

dfawley Nov 25, 2024

purnesh42H Nov 27, 2024

bkane-msft commented Nov 26, 2024

dfawley left a comment

purnesh42H commented Dec 3, 2024

dfawley Dec 5, 2024

purnesh42H Dec 5, 2024

dfawley commented Dec 5, 2024

purnesh42H commented Dec 6, 2024

examples/features/gracefulstop: add example to demonstrate server graceful stop #7865

examples/features/gracefulstop: add example to demonstrate server graceful stop #7865

Conversation

purnesh42H commented Nov 22, 2024 • edited Loading

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

bkane-msft commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkane-msft Nov 22, 2024 • edited Loading

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

purnesh42H commented Nov 25, 2024 • edited Loading

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

bkane-msft commented Nov 26, 2024

dfawley left a comment

Choose a reason for hiding this comment

purnesh42H commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley commented Dec 5, 2024

purnesh42H commented Dec 6, 2024

purnesh42H commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

bkane-msft Nov 22, 2024 •

edited

Loading

purnesh42H commented Nov 25, 2024 •

edited

Loading