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

A little fixed code, request to merge #304

Open
sonnmp opened this issue Jul 5, 2023 · 1 comment
Open

A little fixed code, request to merge #304

sonnmp opened this issue Jul 5, 2023 · 1 comment

Comments

@sonnmp
Copy link

sonnmp commented Jul 5, 2023

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, IServerStreamWriter1 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.<HandleCallAsync>g__AwaitHandleCall|8_0(HttpContextServerCallContext serverCallContext, Method2 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
Copy link
Member

mgravell commented Jul 5, 2023

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

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

No branches or pull requests

2 participants