New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wait for external event with timeout #365

Closed
markheath opened this Issue Jun 27, 2018 · 12 comments

Comments

Projects
None yet
3 participants
@markheath
Contributor

markheath commented Jun 27, 2018

Inspired by Mikhail Shilkov's tweet I wonder if we could simplify the syntax for waiting for an external event with a timeout, as there are multiple lines of boilerplate code required to set this up, and it requires implementers remembering to adhere to the correct patterns (remembering to cancel the CancellationTokenSource and remembering to use DurableOrchestrationContext.CurrentUtcDateTime instead of DateTime.Now).

What would be nice is an overload of WaitForExternalEvent that simply takes a TimeSpan timeout and returns either the received event or something else to indicate a timeout.

I wondered whether this could be simply implemented as an extension method on DurableOrchestrationContext like this:

static class DurableOrchestrationContextExtensions
{
    public static async Task<T> WaitForExternalEvent<T>(this DurableOrchestrationContext ctx, string eventName, TimeSpan timeoutDuration)
    {
        using (var cts = new CancellationTokenSource())
        {
            var timeoutAt = ctx.CurrentUtcDateTime + timeoutDuration;
            var timeoutTask = ctx.CreateTimer(timeoutAt, cts.Token);
            var waitForEventTask = ctx.WaitForExternalEvent<T>(eventName);

            var winner = await Task.WhenAny(waitForEventTask, timeoutTask);
            if (winner == waitForEventTask)
            {
                cts.Cancel();
                return waitForEventTask.Result;
            }
            else
            {
                return default(T);
            }
        }
    }
}

This would greatly simplify the orchestrator code to something like:

var approvalResult = await ctx.WaitForExternalEvent<string>("OrderApprovalResult", TimeSpan.FromMinutes(30));
if (approvalResult == null)
{
   // we timed out
}

However, this doesn't work because of the restrictions on what the orchestrator can do with the await keyword (it doesn't even work if I eliminate the await keyword from the extension method and use ContinueWith instead).

Would it be possible to implement a method like this in the DurableOrchestrationContext class itself? I had a quick look at the code, but it wasn't obvious to me what would need to be done to provide this directly on DurableOrchestrationContext.

My example implementation uses default(T) to indicate timeout, which I suspect would be fine in most cases. It's shame C# doesn't have an idiomatic representation of discriminated unions like F# does, which would be ideal.

I suppose another approach would be to make a reusable sub-orchestrator, which took the external event name and a TimeSpan as input, but I think a method directly on DurableOrchestrationContext would be preferable if it's possible.

@cgillum

This comment has been minimized.

Collaborator

cgillum commented Jun 28, 2018

I think it's a good idea. However, returning default(T) does not seem acceptable IMO because null might be a valid value. For that reason, I think we would need to implement task cancellation.

I haven't tested this yet, but might this work?

public Task<T> WaitForExternalEvent<T>(string name, TimeSpan timeout)
{
    var tcs = new TaskCompletionSource<T>();
    var cts = new CancellationTokenSource();

    var timeoutAt = this.CurrentUtcDateTime + timeout;
    var timeoutTask = this.CreateTimer(timeoutAt, cts.Token);
    var waitForEventTask = this.WaitForExternalEvent<T>(name);

    waitForEventTask.ContinueWith(t =>
    {
        using (cts)
        {
            cts.Cancel();
            if (t.Exception != null)
            {
                tcs.TrySetException(t.Exception);
            }
            else
            {
                tcs.TrySetResult(t.Result);
            }
        }
    });

    timeoutTask.ContinueWith(t =>
    {
        using (cts)
        {
            tcs.TrySetCanceled();
        }
    });

    return tcs.Task;
}
@markheath

This comment has been minimized.

Contributor

markheath commented Jul 2, 2018

I gave this a quick try, but it gave the same error I got with my extension method:

[02/07/2018 13:02:02] Microsoft.Azure.WebJobs.Extensions.DurableTask: Multithreaded execution was detected. This can happen if the orchestrator function code awaits on a task that was not created by a DurableOrchestrationContext method. More details can be found in this article https://docs.microsoft.com/en-us/azure/azure-functions/durable-functions-checkpointing-and-replay#orchestrator-code-constraints.
[02/07/2018 13:02:02] f0d1438af8dd495a83d8d35daee4bd64: Function 'O_ProcessOrder (Orchestrator)' failed with an error. Reason: System.InvalidOperationException: Multithreaded execution was detected. This can happen if the orchestrator function code awaits on a task that was not created by a DurableOrchestrationContext method. More details can be found in this article https://docs.microsoft.com/en-us/azure/azure-functions/durable-functions-checkpointing-and-replay#orchestrator-code-constraints.
@cgillum

This comment has been minimized.

Collaborator

cgillum commented Jul 2, 2018

Ah, I think the ContinueWith calls need TaskContinuationOptions.ExecuteSynchronously.

@markheath

This comment has been minimized.

Contributor

markheath commented Jul 2, 2018

yes, that looks a lot more promising. Also had to move cts.Cancel(); to the bottom of the using block or you always get task canceled

@markheath

This comment has been minimized.

Contributor

markheath commented Jul 2, 2018

Done a bit more testing. This works really nicely. Also can be implemented as an extension method. Here's a working version (and I can make it return default(T) if I prefer not to catch TaskCanceledException):

public static class DurableOrchestrationContextExtensions
{
    public static Task<T> WaitForExternalEvent<T>(this DurableOrchestrationContext ctx, string name, TimeSpan timeout)
    {
        var tcs = new TaskCompletionSource<T>();
        var cts = new CancellationTokenSource();

        var timeoutAt = ctx.CurrentUtcDateTime + timeout;
        var timeoutTask = ctx.CreateTimer(timeoutAt, cts.Token);
        var waitForEventTask = ctx.WaitForExternalEvent<T>(name);

        waitForEventTask.ContinueWith(t =>
        {
            using (cts)
            {
                if (t.Exception != null)
                {
                    tcs.TrySetException(t.Exception);
                }
                else
                {
                    tcs.TrySetResult(t.Result);
                }
                cts.Cancel();
            }
        }, TaskContinuationOptions.ExecuteSynchronously);

        timeoutTask.ContinueWith(t =>
        {
            using (cts)
            {
                tcs.TrySetCanceled();
                // alternatively:
                // tcs.TrySetResult(default(T));
            }
        }, TaskContinuationOptions.ExecuteSynchronously);

        return tcs.Task;
    }
}
@markheath

This comment has been minimized.

Contributor

markheath commented Jul 5, 2018

A couple of questions if I were to issue a PR for this...

  1. Would you prefer it as an extension method on DurableOrchestrationContext or a member directly on DurableOrchestrationContext with an abstract method on DurableOrchestrationContextBase?
  2. Presumably you'd want the version that throws a TaskCanceledException? I agree it is more correct than returning a default(T), but introduces a bit more noise into the calling orchestrator. What about a second overload that took a value of T to return in case of timeout?
@SimonLuckenuik

This comment has been minimized.

SimonLuckenuik commented Jul 5, 2018

  • For 2), what about throwing System.TimeoutException?
  • Could be interesting to add something to workaround the 7 days limitation as well? (creating a new timer if timeout before the expected real timeout that is over 7 days)
@markheath

This comment has been minimized.

Contributor

markheath commented Jul 7, 2018

Yes, TimeoutException might be a more natural fit here, and it's part of netstandard so we could use that.

For the 7 day limitation, I think that should be a separate issue on the CreateTimer method so that when that's fixed, all consumers of CreateTimer including this extension would benefit. CreateTimer already throws an ArgumentException if you attempt to use it with a value too large, so anyone attempting to use longer timeouts.

But I have quickly prototyped something that seems like it might work. I'll post my code on the 7 days limitation issue.

@SimonLuckenuik

This comment has been minimized.

SimonLuckenuik commented Jul 7, 2018

Just need to make sure we use the CreateTimerExtended in WaitForExternalEvent with timeout ;-)

@cgillum

This comment has been minimized.

Collaborator

cgillum commented Jul 7, 2018

For 1) I would prefer adding members to DurableOrchestrationContext and DurableOrchestrationContextBase as you described.

For 2) I agree that TimeoutException is a better choice. I like your idea for a second overload. My only hesitation is that I can't think of any existing .NET API which returns a default value as a result of a timeout, so it might feel unnatural to other developers. I understand the desire to avoid programming by exception though.

@markheath

This comment has been minimized.

Contributor

markheath commented Jul 7, 2018

  1. ok, will do
  2. yes, the lack of a standard way of expressing a discriminated union in C# is a pain. One alternative is for the method to return something like this, which is a pattern that does crop up in other places:
public class WaitForExternalEventResult<T>
{
	private WaitForExternalEventResult(bool timedOut, T externalEvent)
	{
		TimedOut = timedOut;
		result = externalEvent;
	}

	public static WaitForExternalEventResult<T> CreateTimedOut()
	{
		return new WaitForExternalEventResult<T>(true, default(T));
	}
	public static WaitForExternalEventResult<T> CreateWithExternalEvent(T externalEvent)
	{
		return new WaitForExternalEventResult<T>(false, externalEvent);
	}
	private T result;
	public bool TimedOut { get; }
	public T ExternalEvent
	{
		get
		{
			if (TimedOut) throw new InvalidOperationException("Timed Out");
			return result;
		}
	}
}

Would that be preferable?

@cgillum

This comment has been minimized.

Collaborator

cgillum commented Aug 30, 2018

This has been pulled into the v1.6.0 release, which is available now.

@cgillum cgillum closed this Aug 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment