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 :)

[nytian]

Move this logic here, as WriteAsJsonAsync will set response.StatusCode to HttpStatusCode.OK once it completes successfully.

[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?