Azure / azure-functions-dotnet-worker

Azure Functions out-of-process .NET language worker
MIT License
425 stars 182 forks source link

Exception handling middleware not behaving as expected for void or Task return types #1151

Open LockTar opened 1 year ago

LockTar commented 1 year ago

Hi @kshyju,

This is a following up issue for #936.

My last comment indicates that the issue is not fully resolved. In my and @Alan-Hinton opinion the problem is still there for a Task and void return type.

See the same application with extra samples in issue 1 of his sample repository.

The results are:

// Bug???
[Function("ThrowExceptionVoid")]
public void ThrowExceptionVoid(
    [HttpTrigger(AuthorizationLevel.Anonymous, "post", Route = "throw-exception-void")] HttpRequestData req)
    => throw new System.Exception("Test message");

// Bug???
[Function("ThrowExceptionTask")]
public Task ThrowExceptionTask(
    [HttpTrigger(AuthorizationLevel.Anonymous, "post", Route = "throw-exception-task")] HttpRequestData req)
    => throw new System.Exception("Test message");

// Correct expected behavoir
[Function("ThrowExceptionAndReturn")]
public MyResponseType ThrowExceptionAndReturn(
    [HttpTrigger(AuthorizationLevel.Anonymous, "post", Route = "throw-exception-and-return/{returnUsefullData}")] HttpRequestData req,
    bool returnUsefullData)
    => returnUsefullData
        ? throw new System.Exception("Test message")
        : new MyResponseType();

// Correct expected behavoir
[Function("ThrowExceptionHttpResponseData")]
public HttpResponseData ThrowExceptionHttpResponseData(
    [HttpTrigger(AuthorizationLevel.Anonymous, "post", Route = "throw-exception-httpresponsedata")] HttpRequestData req)
    => throw new System.Exception("Test message");

// Correct expected behavoir
[Function("ThrowExceptionTaskHttpResponseData")]
public Task<HttpResponseData> ThrowExceptionTaskHttpResponseData(
    [HttpTrigger(AuthorizationLevel.Anonymous, "post", Route = "throw-exception-task-httpresponsedata")] HttpRequestData req)
    => throw new System.Exception("Test message");
fabiocav commented 1 year ago

Assigning @kshyju for follow up questions/investigation.

kshyju commented 1 year ago

I investigated this. Let me set the context here before I share my findings.

TLDR: For the working function, functions metadata has 2 binding entries, one for req and one for $return. For the non-working one, the $return binding entry is not present in the function metadata. This causes host code to execute different code path.

More details:

Isolated function side

In both cases, the dotnet worker sends back the same invocation response, specifically the http part.

{
  "invocationId": "...",
  "result": { "status": "Success" },
  "returnValue": {
    "http": {
      "headers": { "Content-Type": "application/json; charset=utf-8" },
      "body": { "bytes": "eyJGb29TdGF0dXMiOiJJbnZvY2F0aW9uIGZhaWxlZCEifQ==" },
      "statusCode": "500"
    }
  }
}

Host side handling of invocation response

For the working invocation, the _outputBindings collection has one entry in it. Inside WorkerFunctionInvoker.BindOutputsAsync method, we call FunctionBinding.BindAsync for each of the entries.

image

Now this will cause the HttpBinding.BindAsync to be executed, inside which we build the correct response object(IActionResult instance with status code 500) and add that to the HttpItems collection, which will be later used for returning the http response.

For the non-working case, the _outputBindings collection has no entries since the return type of the function was void. So the HttpBinding.BindAsync method is not being executed.

image

@fabiocav Let's discuss this during the next triage meeting and see whether this behavior should be changed in the future.

kshyju commented 1 year ago

@LockTar The current behavior is by design. If we decide to change this behavior in the future, we will share an update.

LockTar commented 1 year ago

Hi @kshyju Thank you for the explanation. Ok, it's by design but not really what you would expect. Hopefully it's documented that return type should not be Task or void. Maybe this issue should be changed to a feature request?

kshyju commented 1 year ago

@LockTar Thank you for your patience. Agree that the expected behavior you mentioned is valid. The current incorrect behavior is caused by the incorrect metadata generation when the return type is void or Task. We will explore the options to make a fix to handle this case. Will post an update here when that fix is released.

mateo-as commented 10 months ago

Can anyone share a workaround that maybe adds/replaces the outbound binding on the fly? As we have a lot of endpoints that returns void/Task and this issue stops us from upgrading.

Workarounds from issues #765 #936 #530 do not work as they focus on slightly different cases

LockTar commented 7 months ago

@kshyju any news about this issue? Would be great when this is finally fixed.