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

NotSupportedException: 'Cancellation of stream writes is not supported by this gRPC implementation.' #1747

Closed
max-ieremenko opened this issue May 17, 2022 · 9 comments
Labels
enhancement New feature or request

Comments

@max-ieremenko
Copy link

Grpc.Net 2.46.0, Grpc.Core 2.46.1

Is your feature request related to a problem? Please describe.

As an API user, i want to know if IAsyncStreamWriter supports WriteAsync(message, CancellationToken).
I don't want to guess or to catch NotSupportedException thrown by the default implementation.

Describe the solution you'd like

change the default implementation, remove NotSupportedException:

public interface IAsyncStreamWriter<T>
{
    Task WriteAsync(T message, CancellationToken cancellationToken) => WriteAsync(message);
}

Describe alternatives you've considered

add indicator whether the current AsyncStreamWriter supports WriteAsync(message, CancellationToken)

public interface IAsyncStreamWriter<T>
{
    bool CanCancelWriteAsync => false; // default false;

    Task WriteAsync(T message, CancellationToken cancellationToken)
    {
        if (cancellationToken.CanBeCanceled && !CanCancelWriteAsync)
        {
            throw new NotSupportedException("Cancellation of stream writes is not supported by this gRPC implementation.");
        }

        return WriteAsync(message);
    }
}

Additional context

Few examples of WriteAsync in context of Grpc.Net and Grpc.Core WriteAsyncChallenge.zip.

@max-ieremenko max-ieremenko added the enhancement New feature or request label May 17, 2022
@JamesNK
Copy link
Member

JamesNK commented May 24, 2022

Grpc.Core doesn't support WriteAsync(message, CancellationToken). Grpc.AspNetCore does.

@JamesNK
Copy link
Member

JamesNK commented May 24, 2022

Describe alternatives you've considered

add indicator whether the current AsyncStreamWriter supports WriteAsync(message, CancellationToken)\

That means the cancellation token would silently be ignored which isn't good.

@max-ieremenko
Copy link
Author

Grpc.Core doesn't support WriteAsync(message, CancellationToken). Grpc.AspNetCore does.

IAsyncStreamWriter.WriteAsync(CancellationToken) is defined in Grpc.Core is visible to developers and code analyzer.
Resharper makes a suggestion "hey, there's an overload with CancellationToken".

Describe alternatives you've considered

add indicator whether the current AsyncStreamWriter supports WriteAsync(message, CancellationToken)\

That means the cancellation token would silently be ignored which isn't good.

Forcing the user to catch "NotSupportedException" is not good either. What about the alternative "bool CanCancelWriteAsync"?

@JamesNK
Copy link
Member

JamesNK commented May 25, 2022

A property like that is only useful if you have code that could run on either implementation. That doesn't seem like it would be common. But if more people ask for this, then it could be considered.

If you know WriteAsync with a cancellation token isn't supported then just don't call it.

@JamesNK
Copy link
Member

JamesNK commented May 31, 2022

Thank you for your feedback. We're closing this issue as the behavior discussed is by design.

@Falco20019
Copy link

Falco20019 commented Jun 20, 2022

This sadly also did bite us... ReSharper is creating warnings that there is a variant with cancellation token support. Our users just applied it (as it's usually useful) and found out later that this is throwing an exception. So I'm also not too happy with the solution. In my opinion, the macro checking for #if NETSTANDARD2_1_OR_GREATER is misleading.

@NorekZ
Copy link

NorekZ commented Jun 10, 2024

I’d personally be fine with the current state if ReSharper isn’t suggesting filling in the cancellation token from context.

What about adding a setting that would suppress the exception? It’s unwatchable that no team member always ignores this overload (and the suggestion).

@Falco20019
Copy link

@NorekZ We nowadays pass CancellationToken.None to suppress the warning and also make clear that we intentionally not pass in the real token (as this would throw at runtime). Still not 100% happy since there will always be cases where it's not caught in a review (especially with new people joining), but it's the best that we were able to come up with.

Since Grpc.Core is in maintenance-only mode, I am pretty sure that no one will invest any more time on this. I really hope that dotnet/aspnetcore#35077 gets some traction and that we could finally switch to the new implementation.

@NorekZ
Copy link

NorekZ commented Jun 10, 2024

Thanks for suggestion.

I now realized this issue is for deprecated Grpc.Core.

We use Grpc.AspNetCore, where we are observing the same problem.

Eventually I've found problem in our Interceptor, where I missed the new method. I generated IAsyncStreamWriter<TResponse> method stubs using ReSharper, but it didn't generate the one for WriteAsync(TResponse message, CancellationToken cancellationToken) because it has default implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants