Breaking change wishlist for Durable Functions vNext for .NET

[cgillum]

Opening a discussion to propose some breaking API changes that I'd like to see in a future major version of Durable Functions. I decided to make this a discussion in hopes that it would be easier to find and allow folks to chime in if they have other ideas or thoughts.

(NOTE: This is not an announcement of any changes - just keeping track of some changes I'd like to see someday, if we decide it makes sense. Also, it's entirely possible that we could make some of these changes in a non-breaking way.)

• Interfaces vs. abstract classes
  In Durable Functions v2 we moved the "context" objects from interfaces to abstract classes. I regret this move because now every interface change is a breaking change for test code. The C# team did announce support for default interface implementations in C# 8, but they can't be used in .NET Standard 2.0. I'd like us to reconsider a move back to abstract base classes.

• Better instance history purge APIs
  Related to Gracefully handling timeouts on consumption plan while purging orchestrations #882, instance history purge APIs should be more consistent with instance query APIs, and should take a cancellation token as a parameter.

• DurabilityProvider.PurgeHistoryByFilters() should return PurgeHistoryResult instead of int
  This is mainly for API consistency. The DurabilityProivder.PurgeInstanceHistoryByInstanceId() method already returns PurgeHistoryResult.

• IDurableOrchestrationClient.PurgeInstanceHistoryAsync should take a list of OrchestrationRuntimeStatus instead of DurableTask.Core.OrchestrationStatus
  Durable Functions APIs generally are not supposed to expose DurableTask.Core.* APIs directly. The fact that we leak this type through is an inconsistency in the current API design.

• IDurableOrchestrationContext.CreateTimer to remove state parameter:
  This parameter serves no useful purpose, AFAICT.

• Consider removing exceptions from client operations that cannot be raised deterministically
  APIs like RaiseEventAsync will throw an exception if an orchestration is in a non-running state. However, this can be problematic since a lack of an exception does not mean the target orchestration received the event. For example, the status of the orchestration seen by the client at send-time might be Running but it could change to Completed before the orchestration actually receives it. An alternative design would be to never throw an exception. Users would need to rely on server side logs to know whether an event was received.

(More to come as I continue to remember some of the changes I'd like to make - I can also update this list based on recommendations from the community)

[olitomlinson]

Allow orchestrations to return and exit into a runtime status of complete, even when there are activities that are still being awaited.

[cgillum]

This wouldn't be specific to .NET, but I agree it's a good idea as a new default behavior for all languages.

[olitomlinson]

Consider revising how RaiseEventAsync works to move away from throwing exceptions for expected scenarios.

Example 1

Don't throw an invalidOperationException when the orchestration is in non-running state.

azure-functions-durable-extension/src/WebJobs.Extensions.DurableTask/ContextImplementations/DurableClient.cs

Line 662 in d00623c

throw new InvalidOperationException($"Cannot raise event {eventName} for orchestration instance {instanceId} because instance is in {status.OrchestrationStatus} state");

Instead give me a response Tuple (bool accepted, RaiseEventErrorEnum error)

Why?

• An orchestration being complete isn't unexpected scenario, particularly when a 3rd party is raising an event to the orchestration. The 3rd party by virtue can't have a strongly consistent understanding of the runtime status, or even the internal state of the orchestration. So in my head it makes sense to use the syntax (response Tuple) to guide the developer into handling the various failure scenarios at design time. Rather than unexpectedly getting a runtime exception and being unpleasantly surprised.
• Having to do string matching on the message contained in the Exception is just not a stable API to depend upon.

Example 2

Don't throw an ArgumentException when raising an event to an Orchestration that doesn't exist.

azure-functions-durable-extension/src/WebJobs.Extensions.DurableTask/ContextImplementations/DurableClient.cs

Line 633 in d00623c

OrchestrationState status = await GetOrchestrationInstanceStateAsync(taskHubClient, instanceId);

azure-functions-durable-extension/src/WebJobs.Extensions.DurableTask/ContextImplementations/DurableClient.cs

Line 620 in d00623c

throw new ArgumentException($"No instance with ID '{instanceId}' was found.", nameof(instanceId));

Instead use the response Tuple as defined above.

Why?

• Same principle as I described in the previous example - A 3rd party system may not know if an orchestration exists under that Id. For example, In my current system, we have a convention of raising events speculatively - if the orchestration exists and can receive it, happy days - Equally, if the orchestration doesn't exist, then that's also fine and expected - certainly not an Exception.

---

I think with both of the above examples, my way of thinking is now more aligned to the concept of signalling (Durable Entities 1-way signalling is a perfect example) and I see the RaiseEvent API as being in the same vein as SignalEntity - So its quite possible my judgement has a bias and not representative of how others use and perceive RaiseEvent API

[cgillum]

Thanks for these suggestions. I understand the issues you raise but I have some concerns with the proposed solutions. Probably a good opportunity for more design discussion. :)

The first issue is the use of Tuple<bool, RaiseEventErrorEnum>. Generally speaking, it's not good design practice (at least in .NET) to have a public API return a tuple because they aren't very discoverable, requiring folks to go to documentation to learn what the values represent. This is probably debatable, because I know this pattern is pretty common in Go. I just don't see it hardly at all in .NET and might seem unconventional to .NET folks. An improvement on this might be to return just an enum value that also includes a success value in addition to several failure values.

I think you also bring up a common debate about failure code vs. throwing an exception. Again, I'm mostly biased towards existing C#/.NET best practices. One could also use a REST analogy - if I try to post an event to a resource that doesn't exist, is it better to return a 404 or a 200 with something in the response payload that tells me the operation failed? I like the idea of raising an error because it forces you to be considerate of cases where your events can't be delivered - which I think is going to be important for reliable systems. Otherwise, I'd suggest we have a separate API called TryRaiseEventAsync where the API name gives you a more intuitive idea about what the expectations are.

An orchestration being complete isn't unexpected scenario, particularly when a 3rd party is raising an event to the orchestration.

I'm not sure if I agree 100%. It certainly can be unexpected for a variety of workflow scenarios. If we're treating external events like pub/sub-style notifications then I would agree. In fact, I'd suggest not returning any error at all. I'm not sure if that's how external events are commonly used, though.

Having to do string matching on the message contained in the Exception is just not a stable API to depend upon.

Agreed with this sentiment. Can you provide an example where you've needed to do this? I ask because I hadn't considered that someone might do a different compensating action depending on the contents of the exception message.

All of this said, the one part of this API that makes me admittedly uncomfortable (in terms of error handling) is the fact that throwing an exception based on runtime status is actually non-deterministic. For example, you might raise an event to an orchestration that is just about to complete. The RaiseEvent operation will succeed, but the event will be silently discarded because the orchestration finished before the event was handled. This essentially means you can't rely on this exception to tell you whether an event was delivered or not. This makes me wonder if throwing an exception for anything other than the successful enqueuing of the event should be reconsidered.

[olitomlinson]

Fair that Tuple responses aren't a wildly done thing in .NET arenas. I personally prefer return values (tuple or otherwise) for expected or common outcomes. I'm not particularly precious about Tuples, but more so with return values for control-flow.

Regards developer discoverability, Its much easier for a developer to quickly dot into a result Enum to discover all the common/anticipated framework/library failure scenarios, and then introduce compensations at the time of writing the code - It's pretty hard, if not impossible to ignore things like the orchestration lifecycle and runtime status in Durable, so embrace it in the design of the APIs. Its a very opinionated framework, so bring those concerns to the surface.

---

When I initially wrote an integration with RaiseEventAsync, I acknowledged that the return type was Task, so it set my expectations that calling this API method would always result in my event being accepted by the framework, unless there is a catastrophic or transient error. But this isn't the case, because the runtime status has an impact on this.

My argument is that an Orchestration instance which is in the Completed state, isn't catastrophic or transient - it's a legitimate and legal state for an orchestration to be in.

Like I said, In one of my use-cases, I raise events speculatively (which you could say is actually signalling) so throwing an exception in this use-case just doesn't fit, as its making an assumption about my business logic.

<rant>
Same goes for the new v3 CosmosDb SDK - it absolutely winds me up when the SDK throws an exception when I try to do a point-read on a document which doesn't exist, and it does this because someone decided that 404s should always be an exception.

In contrast, if I do a SQL lookup or query with T-SQL, which returns 0 rows, we don't consider this an exception do we? No, because that would be SQL server making an assumption about my business logic, which it can't do. So why is it different for Cosmos and DF - Why do these technologies make assumptions about my business logic? IMO they shouldn't. Just tell me what happened, and I will decide if that needs to be treated as an Exception and propagated.
</rant>

---

Agreed with this sentiment. Can you provide an example where you've needed to do this? I ask because I hadn't considered that someone might do a different compensating action depending on the contents of the exception message.

See my 'Feedback 2' item here #1220

---

All of this said, the one part of this API that makes me admittedly uncomfortable (in terms of error handling) is the fact that throwing an exception based on runtime status is actually non-deterministic. For example, you might raise an event to an orchestration that is just about to complete. The RaiseEvent operation will succeed, but the event will be silently discarded because the orchestration finished before the event was handled. This essentially means you can't rely on this exception to tell you whether an event was delivered or not. This makes me wonder if throwing an exception for anything other than the successful enqueuing of the event should be reconsidered.

I think you're definitely onto something here.

Does it make sense to draw parallel's with Entity Signalling? Signalling makes very little assumptions about the target state of the entity - its entirely fire and forget from my understanding?

Could we say that there should be an alternative/complementary method of raising events to Orchestrations that takes on a more fire-and-forget / speculative semantic? I would personally use this for my use-cases which do not care if the raise event is disgarded on the other side if the Orchestration is Completed.

If not, a simplification of the existing RaiseEvent API with better deterministic semantics would be welcome be an awesome step forward :)

[cgillum]

Entity signaling is a little different mainly because if the entity doesn't exist, it will be created. I don't believe entities really have a concept of being "completed".

I added an item for us to revisit this exception throwing behavior since we can't do it deterministically. I've definitely received some complaints about things like this for other APIs. I think what we'd consider doing is not never throwing an exception except if we fail to enqueue the external event.

[sebastianburckhardt]

Another suggestion for a breaking change: remove the FunctionFailedException wrapping exception when propagating exceptions from activities or suborchestrations back to the calling orchestrations.

This 'exception wrapping' behavior does not provide any value IMO, using an inner exception just obscures the root cause and complicates the exception handling in the orchestrator. For example, it can mean that we don't see the root cause immediately when only the first line of the exception is printed in our tools or in the portal. Seeing the root cause immediately can be a big time saver.

There is one situation in which the FunctionFailedException does make sense, and that is if the runtime is unable to serialize or deserialize the original exception. We should probably still use it for that case.

Also, removing this wrapper would make the behavior consistent with failing entity calls (which already do not use this wrapper).