techsola/ambienttasks

Scoped completion tracking and error handling of tasks as an alternative to fire-and-forget/async void. Easy and test-friendly.

ambienttasks's Issues

Enable tests to report the stack traces of AmbientsTasks entry points for incomplete tasks

Reporting entry point stack traces would make the test assertion message significantly more useful in the case where you aren't sure where or why you should be adding AmbientTasks.WaitAllAsync(); to your test:

The test started ambient tasks but did not wait for them.

using System;
using NUnit.Framework;
using NUnit.Framework.Interfaces;
using Techsola;

[AttributeUsage(AttributeTargets.Assembly | AttributeTargets.Class | AttributeTargets.Method)]
public sealed class WaitForAmbientTasksAttribute : Attribute, ITestAction
{
    public ActionTargets Targets => ActionTargets.Test;

    public void BeforeTest(ITest test)
    {
        AmbientTasks.BeginContext();
    }

    public void AfterTest(ITest test)
    {
        switch (TestContext.CurrentContext.Result.Outcome.Status)
        {
            case TestStatus.Failed:
            case TestStatus.Inconclusive:
            case TestStatus.Skipped:
                return;
        }

        var task = AmbientTasks.WaitAllAsync();
        if (!task.IsCompleted) Assert.Fail("The test started ambient tasks but did not wait for them.");
        task.GetAwaiter().GetResult();
    }
}

Shared global fallback context

I really want to understand this and get the scenario right. It seems bad to rush something in without understanding a use case for it, and it's just as bad to prevent real-world apps from using AmbientTasks out of some kind of idealism. Instead of the strategy of dragging my feet and hoping things magically solve themselves, I'm proposing a compromise. See the final question in the 'Outstanding questions' section.

Goals

The best outcome from my point of view would be to play around with an example app until we come to a full understanding of the problem space.

The next best outcome—with the same disclaimer—is to add a back door with the guidance that we don't fully understand why it exists (until this changes) and that it should only be used as a last resort

The worst outcome is to delay this many more weeks. Nearly the worst outcome is to block the folks that need this from being able to use the NuGet package.

Problem description

/cc @YairHalberstadt who found it necessary in their project to add the following code (https://gitter.im/dotnet/csharplang?at=5d039b76e527d95addd12695):

private static AmbientTaskContext _globalContext = new AmbientTaskContext(null);
private static readonly AsyncLocal<AmbientTaskContext> _localContext = new AsyncLocal<AmbientTaskContext>();

private static AmbientTaskContext CurrentContext => _localContext.Value ?? (_localContext.Value = _globalContext);

/// <summary>
/// <para>
/// Replaces the current async-local scope with a new scope which has its own exception handler and isolated set
/// of tracked tasks.
/// </para>
/// <para>If <paramref name="exceptionHandler"/> is <see langword="null"/>, exceptions will be left uncaught. In
/// the case of tracked <see cref="Task"/> objects, the exception will be rethrown on the synchronization
/// context which began tracking it.
/// </para>
/// </summary>
public static void BeginContext(Action<Exception> exceptionHandler = null)
{
	_localContext.Value = new AmbientTaskContext(exceptionHandler);
}

/// <summary>
/// <para>
/// Sets a global context which will be used by any tasks which do not have a scope.
/// </para>
/// <para>If <paramref name="exceptionHandler"/> is <see langword="null"/>, exceptions will be left uncaught. In
/// the case of tracked <see cref="Task"/> objects, the exception will be rethrown on the synchronization
/// context which began tracking it.
/// </para>
/// </summary>
public static void SetGlobalContext(Action<Exception> exceptionHandler = null)
{
	_globalContext = new AmbientTaskContext(exceptionHandler);
}

Reasons to avoid this

Subverting the purpose of BeginContext

Using a single shared global context takes less thought than thinking about what contexts mean. I'm afraid people will use this when AmbientTasks.BeginContext is actually possible, so I'll make the XML documentation obnoxious.

Shared global locks

There is a possibility of throughput issues due to context-wide locks in AmbientTask methods. If everything is using the same global context, there are now global locks between all AmbientTask calls.
Could these locks be removed? Probably yes, if we have the time and expertise to do something lock-free.

Outstanding questions

1. Has the need for this disappeared along with the five significant fixes and changes that I implemented before publishing the MyGet package? Maybe the whole thing was/is due to a bug.

2. If not, can the use case be clarified? Why is AmbientTasks.Add/Post being called from a place where there was no execution context flow from the AmbientTasks.BeginContext call?

   • Was the AmbientTasks.BeginContext call too late in the program?
   • Is the program using Unsafe methods to opt out of flowing ExecutionContext? Why? (Using AmbientTasks across this boundary seems very bad.)
   • Is the use case only for AmbientTasks.Add/Post, or does AmbientTasks.WaitAllAsync have this same need? I'd like to make WaitAllAsync throw.
   • Can I see an example that I can run? ❤️

3. Is it acceptable to add this API as AmbientTasks.Experimental.SetGlobalFallbackHandler with warnings in the XML docs?

Add convenience overload AmbientTasks.Add(Func<Task>)

This comes up just about all the time. Instead of pushing you towards a local function or a method, a lambda should just be an option so that you don't have to come up with a name:

From the current readme:

private void SomeEventHandler(object sender, EventArgs e)
{
    // Update UI

    AmbientTasks.Add(UpdateInfoAsync());

    async Task UpdateInfoAsync()
    {
        var info = await GetInfoAsync(...);

        // Update UI using info
    }
}

This would be nicer as:

private void SomeEventHandler(object sender, EventArgs e)
{
    // Update UI

    AmbientTasks.Add(() =>
    {
        var info = await GetInfoAsync(...);

        // Update UI using info
    });
}

I wasn't sure at first, but my own usage and examples like in #2 have convinced me.

Convenience overload AmbientTasks.Post(Func<Task>)

One place so far, refactoring from:

private async void EventHandler(object sender, EventArgs e)
{
    await Task.Yield();

    // ...

    await BarAsync();
}

This would be a nice result (proposed):

private void EventHandler(object sender, EventArgs e)
{
    AmbientTasks.Post(async () =>
    {
        // ...

        await BarAsync();
    });
}

This workaround happened to work out, though it would not be as simple if there had been multiple awaits:

private void EventHandler(object sender, EventArgs e)
{
    AmbientTasks.Post(() =>
    {
        // ...

        AmbientTasks.Add(BarAsync());
    });
}

InvalidOperationException: More calls to EndTask than StartTask.

InvalidOperationException: More calls to EndTask than StartTask.

at AmbientTasks.AmbientTaskContext.EndTask()
at AmbientTasks.OnTaskCompleted(System.Threading.Tasks.Task completedTask, object state)
at Se.Util.Wpf.General.AsyncRelayCommand.Execute.__ExecuteInternal|0() Line 115

The code thats calling it looks like this:

		protected override void Execute(T parameter)
		{
			AmbientTasks.Add(ExecuteInternal());

			async Task ExecuteInternal()
			{
				var t = _execute?.Invoke(parameter);
				if (t != null)
				{
					IsRunning = true;
					try
					{
						await t.ConfigureAwait(false);
					}
					catch (Exception ex)
					{
						_log.Error("Async command has faulted", ex);
						throw;
					}
					finally
					{
						IsRunning = false;
					}
				}
			}

		}