Returning null from a grpc interceptor when there is a failure status code

[ziaulhasanhamim]

Why can't return null from an interceptor there is a failure status code.

public class TestInterceptor : Interceptor
{
    public override async Task<TResponse> UnaryServerHandler<TRequest, TResponse>(TRequest request, ServerCallContext context, UnaryServerMethod<TRequest, TResponse> continuation)
    {
        if (Some Logic)
        {
            return await  continuation(request, context);
        }
        context.Status = new Status(StatusCode.Unavailable, "Some information about the issue");
        return null!;
    }
}

I don't think I'm supposed to return a response when there is a failure status code. Or do I? When I return null with failure status code I get this error message

Grpc.Core.RpcException: Status(StatusCode="Cancelled", Detail="No message returned from method.")
at Grpc.AspNetCore.Server.Internal.CallHandlers.UnaryServerCallHandler`3.HandleCallAsyncCore(HttpContext httpContext, HttpContextServerCallContext serverCallContext)

[chwarr]

Throw a RpcException to terminate a call with an error from within an interceptor. With the current APIs, you have to throw an exception. There is no non-exceptional way to terminate a call from within an interceptor.

[ziaulhasanhamim]

Throw a RpcException to terminate a call with an error from within an interceptor. With the current APIs, you have to throw an exception. There is no non-exceptional way to terminate a call from within an interceptor.

What is the motivation for this block of code?

if (response == null)
{
     // This is consistent with Grpc.Core when a null value is returned
     throw new RpcException(new Status(StatusCode.Cancelled, "No message returned from method."));
}

Afaik it's not always the case that there will be a response. There can be no response if something went wrong. That doesn't mean the request was Cancelled. I might have set appropriate status code and error messages using ServerCallContext.

[JamesNK]

As the comment mentions, it is to be consistent with Grpc.Core.

Either a response should be returned, or an RpcException should be thrown. Returning a null response is not supported.

[ziaulhasanhamim]

Is it only to be consistent with Grpc.Core? because I cant make sense of using StatusCode.Cancelled when nothing is returned from an rpc call.

[JamesNK]

Yes

[ziaulhasanhamim]

@JamesNK Checking the status code for ok code would be a good option.

if (response == null && serverCallContext.Status.StatusCode == StatusCode.OK)
{
     throw new RpcException(new Status(StatusCode.Cancelled, "No message returned from method."));
}

because if the status code is not OK that means it has been set manually. So why mess with that then?

[JamesNK]

And what about the code below it that then writes the message? It will error because response is null.

If you want to return a status then throw RpcException. That's how Grpc.Core and Grpc.AspNetCore.Server both work. Adding more ways of doing the same thing isn't a good argument unless there is a reason.

[ziaulhasanhamim]

I think there is a reason. Most of the time one of the reasons for using grpc over rest is performance. Exceptions are not really the performant way because it breaks the normal code flow. Yeah, the performance difference for the exceptions is most of the time is unnoticeable and negligible. But what about other times? Also, exceptions can make code harder to read because of its nature of breaking the flow. For these reasons you can see that the usage of exceptions is really been reduced from Asp.Net to Asp.Net Core. You want to throw an exception when something went wrong in your application(not in the request).

[JamesNK]

Yeah, the performance difference for the exceptions is most of the time is unnoticeable and negligible. But what about other times?

gRPC doesn't use exceptions in typical application flow. If an HTTP request is being canceled, causing gRPC to throw a RpcException, an exception being thrown will have no impact at all on performance.

[ziaulhasanhamim]

Yea. But I was talking about keeping both ways open. Sending status code manually and throwing RpcException

[JamesNK]

@jtattermusch Do you have any thoughts about this? Should a unary method be allowed to set the status on the ServerCallContext, return null, and have the status be sent in the response to the client?

[mayuki]

We use MessagePack for Marshaller instead of protobuf, so it is possible to serialize null.
This means that a Unary method implementation that returns null as TResponse in Method<TRequest, TResponse> can still send a message body.

However, as discussed here, Grpc.AspNetCore.Server will block if it receives null.😢

[ziaulhasanhamim]

Grpc.AspNetCore.Server will block if it receives null

It can be changed. or can't?

[jtattermusch]

We use MessagePack for Marshaller instead of protobuf, so it is possible to serialize null. This means that a Unary method implementation that returns null as TResponse in Method<TRequest, TResponse> can still send a message body.

However, as discussed here, Grpc.AspNetCore.Server will block if it receives null.😢

I think that's a different issue from what's being discussed here though. Generally, we didn't allow messages to be be null since IIRC internally in Grpc.Core, a "null" message is used to signal that there will be no more request/responses in the request / response stream, so supporting "null" messages is a tricky proposition and would require additional work and testing (and we'd want Grpc.Core to stay consistent with grpc-dotnet in terms of what's supported).

[jtattermusch]

So I don't think there's much value in supporting return null! from the handler (regardless whether it's from an interceptor or a regular server side handler), when your intent is to return an error status code.
As suggested earlier, you can just as easily do this:

public class TestInterceptor : Interceptor
{
    public override async Task<TResponse> UnaryServerHandler<TRequest, TResponse>(TRequest request, ServerCallContext context, UnaryServerMethod<TRequest, TResponse> continuation)
    {
        if (Some Logic)
        {
            return await  continuation(request, context);
        }
       throw new RpcException(new Status(StatusCode.Unavailable, "Some information about the issue"));
    }
}

This will do exactly what you need and it won't require returning what's otherwise and illegal return value (null message).

The argument about performance doesn't apply here since throwing and catching an exception has completely negligible overhead compared to starting/handling an RPC. I'd very surprised if you saw any noticeable difference in a benchmark.
And even if the performance mattered, you could simply specialize the interceptor to do

context.Status = new Status(StatusCode.Unavailable, "Some information about the issue");
return MyHelper.PreallocatedEmptyMessageOfTheResponseType;

(and if you're inside a regular server side handler, returning a default instance of a message as a dummy response is even simpler).

The problem with allowing null messages is orthogonal to this issue, as explained above.

I suggest closing this issue as it doesn't really limit the usability.

[JamesNK]

Closing as I don't think the cost of this change is worth any potential benefit. Additionally, the workaround Jan mentioned, to return an empty dummy object, is available.

We'd need to see real-world scenarios where this exception has a noticeable impact on performance to consider this further.