protobuf-net / protobuf-net.Grpc

GRPC bindings for protobuf-net and grpc-dotnet
Other
846 stars 106 forks source link

A little fixed code, request to merge #304

Open sonnmp opened 12 months ago

sonnmp commented 12 months ago

I found that when user cancelled subscribe on duplex streaming, server console show error. It is just cosmetic on console.

12:01:41 fail: Grpc.AspNetCore.Server.ServerCallHandler[6] Error when executing service method 'SubscribeListening'. System.OperationCanceledException: The operation was canceled. at System.Threading.Channels.AsyncOperation1.GetResult(Int16 token) at ProtoBuf.Grpc.ChannelAsyncEnumerableExtensions.AsAsyncEnumerable[T](ChannelReader1 reader, CancellationToken cancellationToken)+MoveNext() in //src/protobuf-net.Grpc/ChannelAsyncEnumerableExtensions.cs:line 34 at ProtoBuf.Grpc.ChannelAsyncEnumerableExtensions.AsAsyncEnumerable[T](ChannelReader1 reader, CancellationToken cancellationToken)+System.Threading.Tasks.Sources.IValueTaskSource<System.Boolean>.GetResult() at ProtoBuf.Grpc.Internal.Reshape.WriteTo[T](IAsyncEnumerable1 reader, IServerStreamWriter`1 writer, CancellationToken cancellationToken) in //src/protobuf-net.Grpc/Internal/Reshape.cs:line 204 at ProtoBuf.Grpc.Internal.Reshape.WriteTo[T](IAsyncEnumerable1 reader, IServerStreamWriter1 writer, CancellationToken cancellationToken) in /_/src/protobuf-net.Grpc/Internal/Reshape.cs:line 204 at Grpc.Shared.Server.ServerStreamingServerMethodInvoker3.Invoke(HttpContext httpContext, ServerCallContext serverCallContext, TRequest request, IServerStreamWriter1 streamWriter) at Grpc.Shared.Server.ServerStreamingServerMethodInvoker3.Invoke(HttpContext httpContext, ServerCallContext serverCallContext, TRequest request, IServerStreamWriter1 streamWriter) at Grpc.AspNetCore.Server.Internal.CallHandlers.ServerStreamingServerCallHandler3.HandleCallAsyncCore(HttpContext httpContext, HttpContextServerCallContext serverCallContext) at Grpc.AspNetCore.Server.Internal.CallHandlers.ServerCallHandlerBase3.g__AwaitHandleCall|8_0(HttpContextServerCallContext serverCallContext, Method`2 method, Task handleCall)

The error point to ChannelAsyncEnumerableExtensions.cs:line 34, which is:

`public static async IAsyncEnumerable AsAsyncEnumerable(this ChannelReader reader, [EnumeratorCancellation] CancellationToken cancellationToken = default)

    {
        while (await reader.WaitToReadAsync(cancellationToken).ConfigureAwait(false))
        {
            while (reader.TryRead(out T? item))
            {
                yield return item!;
            }
        }
    }

` It require a catch here, so I fix it as:

`public static async IAsyncEnumerable AsAsyncEnumerable(this ChannelReader reader, [EnumeratorCancellation] CancellationToken cancellationToken = default)

    {
        bool hasResult = true;
        while (hasResult)
        {
            try
            {
                hasResult = await reader.WaitToReadAsync(cancellationToken).ConfigureAwait(false);
                if (!hasResult)
                    yield break;
            }
            catch (System.OperationCanceledException)
            {
                yield break;
            }

            if (hasResult)
                while (reader.TryRead(out T? item))
                {
                    yield return item!;
                }
        }
    }

` I just check on my server and now the console show error no more. It is somewhat essential because I use console for high severity error. Final, thank for your great work. It help the community a lot

mgravell commented 12 months ago

The code as written is - intentionally - the same as the BCL implementation when it exists, so if we changed this, we'd also need to use the diverged version in all cases, which in turn breaks the virtual; so: before going that route, I'd like to first explore whether we're trying to fix the wrong thing. So; what is the actual repro here? Did the client signal disconnect, or did the client detach without signaling disconnect? (what it means to signal disconnect depends a little on how the client is sending)

Can we see the client part of this conversation? Also, I wonder whether there's something we could/should do in the server binding instead, to catch this exception scenario and instead write an ILogger message at a given id/severity