grpc / grpc-dotnet

gRPC for .NET
Apache License 2.0
4.17k stars 769 forks source link

Should GrpcChannel provide ChannelBase.ShutdownAsync() functionality? #1712

Open jtattermusch opened 2 years ago

jtattermusch commented 2 years ago

Currently there's a discrepancy in GrpcChannel (grpc-dotnet) and Channel (Grpc.Core) behavior when it comes to shutdown.

https://github.com/grpc/grpc/blob/1cd6e69347cbf62a012477fe184ee6fa8f25d32c/src/csharp/Grpc.Core.Api/ChannelBase.cs#L72

https://github.com/grpc/grpc/blob/1cd6e69347cbf62a012477fe184ee6fa8f25d32c/src/csharp/Grpc.Core/Channel.cs#L214

The issue is that when using channels gRPC in .NET, one must specialize the channel shutdown based on knowing the concrete implementation of a given channel (and use Channel.ShutdownAsync() or GrpcChannel.Dispose() based on that).

Could this be solved by e.g. making GrpcChannel.ShutdownAsync() basically behave the same way as Dispose()?

jtattermusch commented 2 years ago

CC @jskeet

JamesNK commented 2 years ago

Does Channel.ShutdownAsync gracefully shutdown? i.e. waits for active calls in progress to finish

It looks like it doesn't.

jtattermusch commented 2 years ago

Does Channel.ShutdownAsync gracefully shutdown? i.e. waits for active calls in progress to finish

It looks like it doesn't.

Nope, it doesn't. The "graceful shutdown" behavior is actually part of Server.cs (which does wait for in-flight calls to finish upon server.ShutdownAsync() and it cancels in flight calls upon server.KillAsync()).

the channel.ShutdownAsync() also doesn't necessarily cancel all the inflight calls (under normal circumstances, you should only shutdown a channel once all the RPCs have finished. If you want RPCs to get automatically cancelled upon channel shutdown in Grpc.Core, you can use Channel.ShutdownToken and explicitly register it with the calls you start.

See the documentation for channelBase.ShutdownAsync() here: https://github.com/grpc/grpc/blob/2badafbc4d246d5165546a61eeb36aa017987d30/src/csharp/Grpc.Core.Api/ChannelBase.cs#L53-L64

Soon5 commented 2 years ago

I would definitely appreciate this, as I was refactoring my solution recently to use "ChannelBase" in the Channel Pool, due to that being the common denominator for the legacy Grpc.Core package and the new implementation. Due to other reasons I don't want to repeat here I am stuck client side to Grpc.Core, so I would appreciate if I now don't have to differentiate between the implementation in my shared lib.... In my opinion, that's what abstraction and polymorphism is for, isn't it :)

jeffijoe commented 1 year ago

It looks like this is affecting the Google Cloud Pub/Sub .NET client, see https://github.com/googleapis/google-cloud-dotnet/issues/10304