Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gracefully stop gRPC servers #401

Merged
merged 3 commits into from
Aug 28, 2018
Merged

gracefully stop gRPC servers #401

merged 3 commits into from
Aug 28, 2018

Conversation

ilgooz
Copy link
Contributor

@ilgooz ilgooz commented Aug 27, 2018

related with #354 (comment)

@ilgooz ilgooz force-pushed the feature/graceful-grpc branch 2 times, most recently from 84909c1 to fccc467 Compare August 27, 2018 13:24
@ilgooz ilgooz requested a review from a team August 27, 2018 13:25
@ilgooz ilgooz force-pushed the feature/graceful-grpc branch 3 times, most recently from dc946ec to a002a51 Compare August 27, 2018 14:19
@ilgooz ilgooz requested review from a team and removed request for a team August 27, 2018 14:20
@ilgooz ilgooz added this to the v1.2.0 milestone Aug 27, 2018
* remove returning error on multiple call to Serve(), callers never should do that.
* remove TestServerServeAlreadyListening test.
@ilgooz ilgooz force-pushed the feature/graceful-grpc branch from a002a51 to 255c268 Compare August 27, 2018 14:37
@ilgooz ilgooz requested a review from a team August 27, 2018 14:51
go startServer(tcpServer)
go startServer(unixServer)

closing := make(chan struct{}, 2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand how this is useful.
Why not doing something like

defer tcpService.Close()
defer unixServer.Close()

instead of having to create this chan that we pass as a parameters of the closeServer we could close the server directly and not in some goroutines. I might be missing something here

Copy link
Contributor Author

@ilgooz ilgooz Aug 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes actually, that channel is needed. Close() will stop accepting incoming gRPC requests and gracefully wait current ones to end. so Close() will block until all accepted gRPC requests are done. We should only exit the program after both Close() calls has returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and the reason why we use a goroutine and a chan is to close both server concurrently and don't force one to wait until another one closes.

antho1404
antho1404 previously approved these changes Aug 28, 2018
@ilgooz ilgooz requested review from NicolasMahe and krhubert August 28, 2018 04:57
NicolasMahe
NicolasMahe previously approved these changes Aug 28, 2018
@antho1404 antho1404 dismissed stale reviews from NicolasMahe and themself via 9c7269d August 28, 2018 10:26
@antho1404 antho1404 merged commit f884ddb into dev Aug 28, 2018
@antho1404 antho1404 deleted the feature/graceful-grpc branch August 28, 2018 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants