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

Update gRPC server to work with recent gRPC versions #3298

merged 3 commits into from Jan 8, 2018


Copy link

commented Jan 7, 2018

When gRPC support initially has been added to Murmur there was no way to get notified when a stream to a client has been cancelled (e.g. by the client or simply network failure). So back then a timer has been used to periodically check all active streams via ServerContext::IsCancelled().

By now there's ServerContext::AsyncNotifyWhenDone(void*) which one can use to get notified of closed/cancelled streams.
In fact, ServerContext::IsCancelled() has been declared unsafe to call unless said method is used (and it does indeed cause Segfaults).

This PR changes Murmur's gRPC server to use AsyncNotifyWhenDone instead of the no longer functional cleanup timer.

Johni0702 added 3 commits Jan 7, 2018
GRPC: remove unsafe calls to IsCancelled
Since AsyncNotifyWhenDone has been added to gRPC, IsCancelled is
never safe to call with the async API unless the done tag has arrived.
Now that AsyncNotifyWhenDone is used to clean up cancelled streams,
calling it isn't needed anyway as any cancelled streams are cleaned
up right after they're known to be cancelled.

@davidebeatrici davidebeatrici requested a review from mkrautz Jan 7, 2018


This comment has been minimized.

Copy link

commented Jan 8, 2018

Looks good. Thanks!

mkrautz approved these changes Jan 8, 2018

@mkrautz mkrautz merged commit 28a8e64 into mumble-voip:master Jan 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
continuous-integration/travis-ci/pr The Travis CI build passed

@Johni0702 Johni0702 deleted the Johni0702:grpc-fixes branch Jan 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
3 participants
You can’t perform that action at this time.