Incorrect nullable annotations on IAsyncEnumerable<T>.Catch()
- 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.