Grpc.Core.RpcException when remote invocation results in BadRequest

[denniszielke]

Expected Behavior

The client sdk should be able to handle HTTP 400 Bad Request and make the request available to the calling application

Actual Behavior

When the app returns a HTTP400 or BadRequest object the dapr client fails with an exception.

Steps to Reproduce the Problem

Small modification of https://github.com/dapr/dotnet-sdk/blob/master/samples/AspNetCore/ControllerSample/Controllers/SampleController.cs to add the following method
to return a BadRequest

/// Do
[HttpPost("do")]
public IActionResult Do(int value)
{
    Console.WriteLine($"Doing {value}"); 
    return BadRequest(new
    {
        value = value
    });
}

When calling the method on the client via

var a = await client.InvokeMethodAsync<object, Account>("routing", "do", data, httpExtension);

we are getting an exception

== APP == DepositUsingServiceInvocation

== APP == invoking

== APP == Unhandled exception. Grpc.Core.RpcException: Status(StatusCode=Internal, Detail="Bad Request")
== APP ==    at Dapr.Client.DaprClientGrpc.MakeGrpcCallHandleError[TResponse](Func`2 callFunc, CancellationToken cancellationToken) in /dotnet-sdk/src/Dapr.Client/DaprClientGrpc.cs:line 585

It seems there is no handling in the client sdk https://github.com/dapr/dotnet-sdk/blob/master/src/Dapr.Client/DaprClientGrpc.cs#L584

RELEASE NOTE: N/A

[akkie]

Currently the grpc-dotnet SDK doesn't support the gRPC rich error model. If this would be supported, the calling code could catch the RpcException and extract the needed information from it.

[amanbha]

@denniszielke SDK is not crashing, exception is being thrown from the gRPC call, Calling code can catch the exception and handle it accordingly.

[akkie]

@amanbha The issue is that we need also the response for non 2xx status codes and currently the GrpcException doesn't contain any information about the response. So currently it's not possible to react to failing requests based on the response send from the HTTP API. So it would be great if the dotnet-sdk would support the rich error model mentioned in my previous comment.

[amanbha]

@akkie yes, I agree, thanks for pointing to the issue grpc-dotnet which needs to be fixed before users can examine it in the exception.

[denniszielke]

yes good point. Our goal would be to handle the request in the app for which we need access to the header, request details and body.

[amanbha]

@denniszielke Can this issue be closed, once its fixed in grpc-dotnet is closed, the information will be available in the exception

[akkie]

@amanbha The Grpc.Core.RpcException class has a property trailers which can contain a list of metadata entries. A metadata entry can contain bytes which makes it able to transport the needed error details like response body and response headers. The HTTP status code can be transported in the status property of the RpcException.

The AsyncUnaryCall which is used by the DaprClientGrpc implementation does handle the trailers from the gRPC request.

Dapr explicitly ignores trailers for HTTP responses. Maybe here we can set also trailers with the required response data?

[amanbha]

Thanks @akkie.
@yaron2 , @youngbupark Is Dapr runtime passing correct information as explained above in gRPC when the target service to be invoked is http (here client is calling a http service over Dapr's gRPC api)

[yaron2]

Thanks @akkie.
@yaron2 , @youngbupark Is Dapr runtime passing correct information as explained above in gRPC when the target service to be invoked is http (here client is calling a http service over Dapr's gRPC api)

I think I may have noticed an issue there, will look into this.

[youngbupark]

Thanks @akkie.
@yaron2 , @youngbupark Is Dapr runtime passing correct information as explained above in gRPC when the target service to be invoked is http (here client is calling a http service over Dapr's gRPC api)

@akkie we ignored trailers for grpc -> http invocation because fasthttp doesn't support trailers.

https://github.com/dapr/dapr/blob/a37f65051e761e7df2c63d22c9c22ad348ad95bc/pkg/messaging/v1/util.go#L318

When we look at this code, we translate all status code and error message body as detail status property. grpc official csharp lib example shows how to get status detail - https://github.com/avinassh/grpc-errors/blob/b84393d1d7fd97e1d6d82bfdd6b9f154171475ee/csharp/Hello/HelloClient/Program.cs#L28

[youngbupark]

Thanks @akkie.
@yaron2 , @youngbupark Is Dapr runtime passing correct information as explained above in gRPC when the target service to be invoked is http (here client is calling a http service over Dapr's gRPC api)

@akkie we ignored trailers for grpc -> http invocation because fasthttp doesn't support trailers.

https://github.com/dapr/dapr/blob/a37f65051e761e7df2c63d22c9c22ad348ad95bc/pkg/messaging/v1/util.go#L318

When we look at this code, we translate all status code and error message body as detail status property. grpc official csharp lib example shows how to get status detail - https://github.com/avinassh/grpc-errors/blob/b84393d1d7fd97e1d6d82bfdd6b9f154171475ee/csharp/Hello/HelloClient/Program.cs#L28

Please ignore this. looks like the official csharp lib error example is wrong.

When it comes to adding request body to trailer metadata, there is the metadata size limit which is 4KB for the entire metadata. putting request body to metadata would not be the proper solution.

[yaron2]

IMO we need to make sure we return the following information to the caller when invoking HTTP services, uniformly:

1. Response text
2. Status code
3. Headers

[amanbha]

@yaron2 and @youngbupark, this looks like a change in runtime for service invocation building block, should I transfer it to dapr repo?