dotnet / dotnet/reactive

Incorrect nullable annotations on IAsyncEnumerable<T>.Catch()

Open
#1,472 1 comment 0 reactions 0 assignees View on GitHub
[area] Ix
Dominant language
C#
Stars
7.2k
Forks
798
PR merge metrics
No merged PRs in 30d

Description

#### Bug

Within `System.Interactive.Async`...

Each of the three overloads of `AsyncEnumerableEx.Catch()` accepts a handler delegate that indicates it should not be given a `null` followup sequence, in the event of a caught exception. However, actually passing null is allowable. It seems plausible that allowing nulls here may not have been the original intention, but it actually is an important use-case.

I found myself getting server logs littered with `TaskCancelledException` and `OperationCancelledException` errors, when using GRPC with `IAsyncEnumerable` for server-to-client streams. In my case, streams are infinite, so it's entirely normal for a client to submit a cancellation, to stop listening when it no longer needs to. I figured I could use the `.Catch()` operator to swallow these exceptions, as follows...

```cs
sequence
.Catch((OperationCancelledException _) => AsyncEnumerable.Empty())
```

However, this doesn't work, because after catching the exception, the `.Catch()` operator passes the already-cancelled token to the empty, which immediately throws a new one.

```
fail: Grpc.AspNetCore.Server.ServerCallHandler[6]
Error when executing service method 'Observe'.
System.OperationCanceledException: The operation was canceled.
at System.Threading.CancellationToken.ThrowOperationCanceledException()
at System.Linq.AsyncEnumerable.EmptyAsyncIterator`1.GetAsyncEnumerator(CancellationToken cancellationToken) in /_/Ix.NET/Source/System.Linq.Async/System/Linq/Operators/Empty.cs:line 52
at System.Runtime.CompilerServices.ConfiguredCancelableAsyncEnumerable`1.GetAsyncEnumerator()
at System.Linq.AsyncEnumerableEx.g__Core|6_0[TSource,TException](IAsyncEnumerable`1 source, Func`2 handler, CancellationToken cancellationToken)+MoveNext() in /_/Ix.NET/Source/System.Interactive.Async/System/Linq/Operators/Catch.cs:line 74
at System.Linq.AsyncEnumerableEx.g__Core|6_0[TSource,TException](IAsyncEnumerable`1 source, Func`2 handler, CancellationToken cancellationToken)+System.Threading.Tasks.Sources.IValueTaskSource.GetResult()
at ProtoBuf.Grpc.Internal.Reshape.WriteTo[T](IAsyncEnumerable`1 reader, IServerStreamWriter`1 writer, CancellationToken cancellationToken) in /_/src/protobuf-net.Grpc/Internal/Reshape.cs:line 134
at ProtoBuf.Grpc.Internal.Reshape.WriteTo[T](IAsyncEnumerable`1 reader, IServerStreamWriter`1 writer, CancellationToken cancellationToken) in /_/src/protobuf-net.Grpc/Internal/Reshape.cs:line 134
at Grpc.Shared.Server.ServerStreamingServerMethodInvoker`3.Invoke(HttpContext httpContext, ServerCallContext serverCallContext, TRequest request, IServerStreamWriter`1 streamWriter)
at Grpc.Shared.Server.ServerStreamingServerMethodInvoker`3.Invoke(HttpContext httpContext, ServerCallContext serverCallContext, TRequest request, IServerStreamWriter`1 streamWriter)
at Grpc.AspNetCore.Server.Internal.CallHandlers.ServerStreamingServerCallHandler`3.HandleCallAsyncCore(HttpContext httpContext, HttpContextServerCallContext serverCallContext)
at Grpc.AspNetCore.Server.Internal.CallHandlers.ServerCallHandlerBase`3.g__AwaitHandleCall|8_0(HttpContextServerCallContextserverCallContext, Method`2 method, Task handleCall)
```

As far as I can tell, there's no other way within the library to deal with exceptions coming out of a cancellation token.

I can see [here](https://github.com/dotnet/reactive/blob/9329157592c13e97ce2d3251c91b2871aed875c9/Ix.NET/Source/System.Interactive.Async/System/Linq/Operators/Catch.cs#L72) that the operator is explicitly performing a null-check, and simply ends the sequence with no followup, if no followup sequence is given.

The simple change seems to be to simply mark the followup sequence as nullable, and keep all the functional code the same.

```cs
public static IAsyncEnumerable Catch(
this IAsyncEnumerable source,
Func> handler)
where TException : Exception
```

Alternatively, a new overload of the operator could be added that takes a `TException` argument, but no `handler` to produce a followup sequence.

This seems very closely related to #1207, but if I'm reading it correctly, I don't think this is a duplicate. In this case, it's not the `Catch()` operator that's performing a token check, but the `Empty()` operator, which, according to @bartdesmet 's proposal, in #1207, is the intended behavior for a "generator" operator. Even with #1207 fixed, this would still be an issue.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.