Add WaitForCompletionOrCreateCheckStatusResponseAsync to Microsoft.Azure.Functions.Worker.DurableTaskClientExtensions

[dixonte]

My azure functions rely upon WaitForCompletionOrCreateCheckStatusResponseAsync from the in-proc model, but this feature was missing from the isolated model. I have done my best to create an implementation that not only fulfills my needs but could be used by others. Documentation is the same as the in-proc model.

Pull request checklist

• My changes do not require documentation changes
  • Otherwise: Documentation PR is ready to merge and referenced in pending_docs.md
• My changes should not be added to the release notes for the next release
  • Otherwise: I've added my notes to release_notes.md
• My changes do not need to be backported to a previous version
  • Otherwise: Backport tracked by issue/PR #issue_or_pr
• I have added all required tests (Unit tests, E2E tests)
• My changes do not require any extra work to be leveraged by OutOfProc SDKs
  • Otherwise: That work is being tracked here: #issue_or_pr_in_each_sdk
• My changes do not change the version of the WebJobs.Extensions.DurableTask package
  • Otherwise: major or minor version updates are reflected in /src/Worker.Extensions.DurableTask/AssemblyInfo.cs
• My changes do not add EventIds to our EventSource logs
  • Otherwise: Ensure the EventIds are within the supported range in our existing Windows infrastructure. You may validate this with a deployed app's telemetry. You may also extend the range by completing a PR such as this one.
• My changes should be added to v3.x branch.
  • Otherwise: This change only applies to Durable Functions v2.x and will not be merged to branch v3.x.

[dixonte]

@microsoft-github-policy-service agree company="pitt&sherry"
@microsoft-github-policy-service agree company="Microsoft"

[MarioMajcicaAtABNAMRO]

This would be very handy to have merged and released

[MarioMajcicaAtABNAMRO]

@dixonte One thing though, in InProc we used to deal with HttpResponseMessage as return type, I see you went for HttpResponseData. Any particular reason for that?

[dixonte]

@MarioMajcicaAtABNAMRO That's what the main library was using for isolated worker.

[MarioMajcicaAtABNAMRO]

@MarioMajcicaAtABNAMRO That's what the main library was using for isolated worker.

For the InProcess mode, lib is using the objects from ASP.NET Core integration. Which by the way are supported also in the isolated mode. https://learn.microsoft.com/en-us/azure/azure-functions/dotnet-isolated-process-guide?tabs=windows#http-trigger

HTTP triggers allow a function to be invoked by an HTTP request. There are two different approaches that can be used:

An ASP.NET Core integration model that uses concepts familiar to ASP.NET Core developers
A built-in model, which doesn't require extra dependencies and uses custom types for HTTP requests and responses. This approach is maintained for backward compatibility with previous .NET isolated worker apps.

Seems that ASP.NET Core integration model is the way to go forward if no compatibility issues are present. Should there be 2 versions of this extension, to deal with both?

[dixonte]

@MarioMajcicaAtABNAMRO To me it makes sense to use HttpResponseData as that is part of the Microsoft.Azure.Functions.Worker.Http namespace, which you are most likely going to be using if you're using DurableTaskClientExtensions from the Microsoft.Azure.Functions.Worker namespace. Though admittedly I just followed suit with the class as it existed before my changes; there are other functions returning HttpResponseData in this class prior to my changes.

[MarioMajcicaAtABNAMRO]

@MarioMajcicaAtABNAMRO To me it makes sense to use HttpResponseData as that is part of the Microsoft.Azure.Functions.Worker.Http namespace, which you are most likely going to be using if you're using DurableTaskClientExtensions from the Microsoft.Azure.Functions.Worker namespace. Though admittedly I just followed suit with the class as it existed before my changes; there are other functions returning HttpResponseData in this class prior to my changes.

The fact is that the built in model seems there for legacy reasons:

https://learn.microsoft.com/en-us/azure/azure-functions/dotnet-isolated-process-guide?tabs=windows#http-trigger

[cgillum]

I'm okay with depending on HttpRequestData and HttpResponseData for now as part of this PR. I think there needs to be a separate effort for us to align with the ASP.NET Core integration model, which might be a larger and more complex work item.

[nytian]

Hey @dixonte thanks for your contributions and for opening this PR! To help get this merged, would you mind if I make some direct changes here? I’d like to add unit tests and merge the latest updates from our repo. Unfortunately, since this is a forked repo, I don’t have access to create a new branch, so making updates here would be the most efficient way. Thanks again for your understanding!

[dixonte]

@nytian Absolutely, please do. All I want is for these changes to make it to an official build, so go for it.

[nytian]

@dixonte, I removed this section of code from this commit because it wasn’t functioning as expected. I’m not an expert on HTTP forwarding, so perhaps @jviau , who left the comments, might have additional context on this.

Anyways thanks again for this contribution. I think opening another PR might be better, as this change is somewhat separate from adding the WaitForCompletionOrCreateCheckStatusResponseAsync API, and separating it could also make the PR merge faster. If you're interested, we could address it there :)

[dixonte]

@dixonte, I removed this section of code from this commit because it wasn’t functioning as expected. I’m not an expert on HTTP forwarding, so perhaps @jviau , who left the comments, might have additional context on this.

I think I had some issues getting the returned URIs to work when deployed to a linux host in Azure without those changes, that's why I bundled them in this PR. E.g. the status URI would have http://localhost:XXXXX at the start instead of the expected https://YYYYY.azurewebsites.net, somewhat defeating the entire purpose.

What do you mean, it wasn't functioning as expected, @nytian?

[nytian]

@dixonte thanks for the follow up. I tested this locally and noticed that no baseUrl was returned with the change. I haven’t yet tested it in an Azure environment, so I’m not sure if this is due to a lack of forwarding configuration in the local setup or if it’s something specific to Azure. Let me look into this further, and any additional insights you might have would be much appreciated!

[dixonte]

@nytian Nothing else comes to mind right now. If I get a moment I'll check my own solution locally. Hard to imagine why baseUrl would be blank without stepping through the code.

[jviau]

Regarding the base URI calculation / rewrites. I don't think it is a good idea for this code base to incorporate any of that calculation. There is too much nuance, edge cases, different ways to do it that it is not worth having durable team try and maintain it.

Instead, what I recommend is:

1. First see what happens when using the AspNetCore integration. I believe this integration is supposed to handle HTTP forwarding details and update the request appropriately.
2. Outside of AspNetCore integration, how difficult is it to call this method without the URL calculation, and then manually update the URIs yourself afterwards?
3. If number 2 is not feasible, can we add an overload which accepts the base URI?

Overall though I am not a fan of this API. It does not follow the Long-running-operation polling pattern. The end caller should be calling a get-status API to monitor if it is done, and then when it is that API should return either the location, they can get the final result from, or the final result itself. It is not recommended to have an HTTP endpoint which waits for some duration for completion and then gives up, returning the status at that time.

I don't recommend this become part of the API. I understand it was part of in-proc, but this is a good time to drop the API by keeping it from being included in out-of-proc.

If you want to implement something yourself, you could add this helper to your project:

public static async Task<OrchestrationMetadata> WaitOrGetStatusAsync(this DurableTaskClient client, string instanceId, TimeSpan timeout)
{
    using CancellationTokenSource cts = new(timeout);

    try
    {
        return await client.WaitForInstanceCompletionAsync(instanceId, cts.Token);
    }
    catch (OperationCanceledException)
    {
        return (await client.GetInstanceAsync(instanceId))!;
    }
}

// check if `OrchestrationMetadata` is terminal. If it is return its payload. If not, call `CreateCheckStatusResponseAsync`

[cgillum]

I'm fine with keeping the forwarding support for methods that use HttpRequestData and HttpResponseData. There are enough cases where I've seen a need for this that I think it's worth including. However, I'm okay with excluding it from methods that work with the AspNetCore integration since that integration should be able to handle this automatically, as @jviau said.

I don't recommend this become part of the API. I understand it was part of in-proc, but this is a good time to drop the API by keeping it from being included in out-of-proc.

While I understand the sentiment, I disagree with excluding it from out-of-proc. I think it's more important that we remove obstacles preventing developers from migrating from in-proc to out-of-proc than having a perfect API surface area. I think this is a design wart we'll just have to continue living with. I'm willing to reconsider this in the case of AspNetCore integration, however (which is something our APIs don't have any support for currently, IIRC).

[cgillum]

I'm good with these changes if we can add tests for the forwarding headers.

[cgillum]

LGTM!