Serve() should not return error on Stop() or GracefulStop() #1485

sunshangpp · 2017-08-28T20:25:25Z

As #1372 reported, currently Serve() function returns non-nil error when Stop() or GracefulStop() is called.

This is because both Stop and GracefulStop close the listener (net.Listener) directly, which is still being blocked on the Accept() call in Serve(). We can use a quit channel to indicate that the grpc server is being stopped on demand and return nil instead of an error.

googlebot · 2017-08-28T20:25:28Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

sunshangpp · 2017-08-28T20:27:13Z

I signed it!

googlebot · 2017-08-28T20:27:15Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot · 2017-08-28T20:30:56Z

CLAs look good, thanks!

menghanl · 2017-09-21T18:42:12Z

Sorry for the late reply, and thanks for the change.

The changes in this PR look good.
But #1372 is actually about making Serve() not return when GracefulStop() is called, and block until the server is really stopped.
It's not just to make Serve() return nil error or not.
Do you want to try to solve this in another PR?

sunshangpp · 2017-09-26T04:47:40Z

Hi @menghanl thanks for the reply!

Originally I hesitated to block Serve because this can be easily done in the app code so we have more flexibility. Also, as soon as server.lis is closed, the gRPC server stops Serving new requests, so semantically it's not really serving anymore. But I can totally see the benefits of blocking as well, I'll make the change.

I think it'll be easier for me to do it in the same PR, I will make a new commit!

menghanl

Can you add some tests for this new behavior?
Thanks!

menghanl · 2017-10-03T22:41:56Z

server.go

+
+			// If Stop or GracefulStop is called, block until they are done and return nil
+			select {
+			case <-s.quit:


A server can actually Serve on multiple listeners. This only works in the case of one Serve.

ah good catch, will fix!

@menghanl Fixed by using a sync.Once to close the channels, this way when s.quit or s.done is closed, all go-routines running Serve() will be unblocked.

sunshangpp · 2017-10-09T15:29:27Z

server_test.go

+
+	err = server.Serve(lis)
+	if err != nil {
+		t.Fatalf("Serve() returned non-nil error on GracefulStop: %v", err)


hey @menghanl I only added test for testing Serve() should return nil on GracefulStop().

I found it tricky to actually test the order of Serve and GracefulStop, the only way I can think of is to make both functions send a message to a shared channel when they return, and compare the order of the messages in the channel, but this requires modifying Serve and GracefulStop themselves.

One way to test this is to make GracefulStop() block by having a pending streaming RPC, and check that Serve() doesn't return.
We have end2end tests that does similar things.

sunshangpp · 2017-10-18T14:45:39Z

hey @menghanl mind taking another look at this? thanks!

thelinuxfoundation · 2017-10-18T14:45:41Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

User @sunshangpp isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
The Linux Foundation CLA GitHub bot

sunshangpp · 2017-10-18T16:00:49Z

I signed

menghanl

LGTM.

menghanl · 2017-10-18T22:26:13Z

server_test.go

+
+	err = server.Serve(lis)
+	if err != nil {
+		t.Fatalf("Serve() returned non-nil error on GracefulStop: %v", err)


One way to test this is to make GracefulStop() block by having a pending streaming RPC, and check that Serve() doesn't return.
We have end2end tests that does similar things.

menghanl · 2017-10-18T22:29:03Z

Thanks for the changes. I'm merging this PR as it has been delayed for a long time.
If you are still interested, you can add the test I mentioned in another PR.
Thanks!

sunshangpp force-pushed the gracefulstop-without-error branch from 23d8a89 to 1bc060d Compare August 28, 2017 20:30

dfawley added the Type: Bug label Aug 31, 2017

lyuxuan assigned menghanl Aug 31, 2017

sunshangpp force-pushed the gracefulstop-without-error branch 2 times, most recently from b8d6e06 to fbf482e Compare September 26, 2017 05:04

menghanl reviewed Oct 3, 2017

View reviewed changes

sunshangpp force-pushed the gracefulstop-without-error branch from fbf482e to 939f1c7 Compare October 9, 2017 15:18

sunshangpp commented Oct 9, 2017

View reviewed changes

sunshangpp force-pushed the gracefulstop-without-error branch 3 times, most recently from f756b42 to e2c3696 Compare October 10, 2017 01:21

sunshangpp added 3 commits October 18, 2017 13:02

Serve() should not return error on Stop() or GracefulStop()

0e8aa97

Block Serve() until Stop() or GracefulStop() returns

6621144

Add test for GracefulStop()

388d7a2

sunshangpp force-pushed the gracefulstop-without-error branch from e2c3696 to 388d7a2 Compare October 18, 2017 17:02

menghanl approved these changes Oct 18, 2017

View reviewed changes

menghanl merged commit 473b142 into grpc:master Oct 18, 2017

menghanl added this to the 1.8 Release milestone Nov 7, 2017

btoews mentioned this pull request Dec 7, 2017

GracefulStop doesn't wait for RPC to finish #1110

Closed

gjnoonan mentioned this pull request Dec 8, 2017

Serve() returns before GracefulStop() #1372

Closed

dfawley mentioned this pull request Feb 13, 2018

race between Server.Serve and Server.Stop #1861

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serve() should not return error on Stop() or GracefulStop() #1485

Serve() should not return error on Stop() or GracefulStop() #1485

sunshangpp commented Aug 28, 2017

googlebot commented Aug 28, 2017

sunshangpp commented Aug 28, 2017

googlebot commented Aug 28, 2017

googlebot commented Aug 28, 2017

menghanl commented Sep 21, 2017

sunshangpp commented Sep 26, 2017

menghanl left a comment

menghanl Oct 3, 2017

sunshangpp Oct 5, 2017

sunshangpp Oct 9, 2017

sunshangpp Oct 9, 2017

menghanl Oct 18, 2017

sunshangpp commented Oct 18, 2017

thelinuxfoundation commented Oct 18, 2017

sunshangpp commented Oct 18, 2017

menghanl left a comment

menghanl Oct 18, 2017

menghanl commented Oct 18, 2017

Serve() should not return error on Stop() or GracefulStop() #1485

Serve() should not return error on Stop() or GracefulStop() #1485

Conversation

sunshangpp commented Aug 28, 2017

googlebot commented Aug 28, 2017

sunshangpp commented Aug 28, 2017

googlebot commented Aug 28, 2017

googlebot commented Aug 28, 2017

menghanl commented Sep 21, 2017

sunshangpp commented Sep 26, 2017

menghanl left a comment

Choose a reason for hiding this comment

menghanl Oct 3, 2017

Choose a reason for hiding this comment

sunshangpp Oct 5, 2017

Choose a reason for hiding this comment

sunshangpp Oct 9, 2017

Choose a reason for hiding this comment

sunshangpp Oct 9, 2017

Choose a reason for hiding this comment

menghanl Oct 18, 2017

Choose a reason for hiding this comment

sunshangpp commented Oct 18, 2017

thelinuxfoundation commented Oct 18, 2017

sunshangpp commented Oct 18, 2017

menghanl left a comment

Choose a reason for hiding this comment

menghanl Oct 18, 2017

Choose a reason for hiding this comment

menghanl commented Oct 18, 2017