Exception handling middleware fails for void return type

Alan-Hinton commented 1 year ago

See #936, which was closed despite not being fixed.

If you use the sample exception handling middleware and have an http triggered functions with void or bare Task return type that throws an exception the request results in a 200 response and the whole http response is returned as json.

I have created a minimal example which shows the error, as well as showing how returning a different return type works fine.

i.e. the request to the void function returns the wrong status code and response

C:\Code>curl "http://localhost:7071/api/throw-exception" -H "accept: */*" -d "" -v
*   Trying 127.0.0.1:7071...
* Connected to localhost (127.0.0.1) port 7071 (#0)
> POST /api/throw-exception HTTP/1.1
> Host: localhost:7071
> User-Agent: curl/7.83.1
> accept: */*
> Content-Length: 0
> Content-Type: application/x-www-form-urlencoded
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Length: 208
< Content-Type: application/json; charset=utf-8
< Date: Wed, 19 Oct 2022 19:33:06 GMT
< Server: Kestrel
<
{"method":"","query":{},"statusCode":"500","headers":{"Content-Type":"application/json; charset=utf-8"},"enableContentNegotiation":false,"cookies":[],"body":"eyJGb29TdGF0dXMiOiJJbnZvY2F0aW9uIGZhaWxlZCEifQ=="}* Connection #0 to host localhost left intact

whereas the endpoint that has a non-void return type gives the correct 500 response

C:\Code>curl "http://localhost:7071/api/throw-exception-and-return/true" -H "accept: */*" -d "" -v
*   Trying 127.0.0.1:7071...
* Connected to localhost (127.0.0.1) port 7071 (#0)
> POST /api/throw-exception-and-return/true HTTP/1.1
> Host: localhost:7071
> User-Agent: curl/7.83.1
> accept: */*
> Content-Length: 0
> Content-Type: application/x-www-form-urlencoded
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 500 Internal Server Error
< Content-Type: application/json; charset=utf-8
< Date: Wed, 19 Oct 2022 19:35:16 GMT
< Server: Kestrel
< Transfer-Encoding: chunked
<
{"FooStatus":"Invocation failed!"}* Connection #0 to host localhost left intact

I have tried fiddling with the middleware to get this working, but there doesn't seem to be a way. So I think this is a bug in the underlying framework. There doesn't seem to be any reason I shouldn't be able to have a request with a void return type.

kshyju commented 4 months ago

Notes from investigation:

api/throw-exception-and-return/true -> throws exception from function. Method has a POCO return type.
api/throw-exception -> throws exception from function code. Method has void as return type for http trigger.

In both cases (api/throw-exception-and-return/true & api/throw-exception), the worker is returning the same invocation response back to host. The host code which handles the invocation response is executing differently for these 2 cases.

The function metadata generated for the 2 functions are as below. note that the "throw-exception" function is missing the $return binding.

public Task<ImmutableArray<IFunctionMetadata>> GetFunctionMetadataAsync(string directory)
{
    var metadataList = new List<IFunctionMetadata>();
    var Function0RawBindings = new List<string>();
    Function0RawBindings.Add(@"{""name"":""req"",""type"":""httpTrigger"",""direction"":""In"",""authLevel"":""Anonymous"",""methods"":[""get""],""route"":""throw-exception""}");

    var Function0 = new DefaultFunctionMetadata
    {
        Language = "dotnet-isolated",
        Name = "ThrowException",
        EntryPoint = "MiddlewareStatusCode.ExceptionController.ThrowException",
        RawBindings = Function0RawBindings,
        ScriptFile = "MiddlewareStatusCode.dll"
    };
    metadataList.Add(Function0);
    var Function1RawBindings = new List<string>();
    Function1RawBindings.Add(@"{""name"":""req"",""type"":""httpTrigger"",""direction"":""In"",""authLevel"":""Anonymous"",""methods"":[""get""],""route"":""throw-exception-and-return/{returnUsefullData}""}");
    Function1RawBindings.Add(@"{""name"":""$return"",""type"":""http"",""direction"":""Out""}");

    var Function1 = new DefaultFunctionMetadata
    {
        Language = "dotnet-isolated",
        Name = "ThrowExceptionAndReturn",
        EntryPoint = "MiddlewareStatusCode.ExceptionController.ThrowExceptionAndReturn",
        RawBindings = Function1RawBindings,
        ScriptFile = "MiddlewareStatusCode.dll"
    };
    metadataList.Add(Function1);

    return Task.FromResult(metadataList.ToImmutableArray());
}

The WorkerFunctionInvoker.BindOutputAsync relies on the output bindings from the metadata to handle the response(output binding data to be specific) after invocation.

https://github.com/Azure/azure-functions-host/blob/94bd90c9ef10c6be61ff9232c189f5721a512152/src/WebJobs.Script/Description/Workers/WorkerFunctionInvoker.cs#L161-L187

For the "throw-exception-and-return" function, there is one output binding which causes the HttpBinding.BindAsync method to be invoked. For throw-exception case, since there are no output bindings, the FunctionBinding.BindAsync is not engaged at all, causing the response to be eventually serialized and sent out with a 200 status code.

Next step is to discuss how /where to handle this.

Option1

Update metadata generation to create the $return binding regardless of functions return type, if the function is Http trigger. What are the impacts of this change?

Option2

Update host code to use a fallback HttpBinding.BindAsync For http trigger invocation, this could be the HttpBinding binder.

fabiocav commented 4 months ago

Discussed this in triage. We'll move forward with option 1 and keep this issue open to track the work.

kshyju commented 3 months ago

@Alan-Hinton

Version 1.17.4 of Worker.Sdk includes the fix for the issue. Please upgrade your package to this version, and feel free to reach out if you encounter any further issues.

Microsoft.Azure.Functions.Worker.Sdk 1.17.4

Azure / azure-functions-dotnet-worker