[API Proposal]: Add async overload of ChangeToken.OnChange

[davidfowl]

Background and motivation

Today it's not possible to use ChangeToken.OnChange to execute asynchronous logic before re-subscribing for new updates. The current API has a Action<T> based callback only (https://docs.microsoft.com/en-us/dotnet/api/microsoft.extensions.primitives.changetoken.onchange?view=dotnet-plat-ext-6.0). We need an async overload to run async logic so people don't end up using async void or blocking when using this helper.

API Proposal

namespace Microsoft.Extensions.Primitives;

public static class ChangeToken
{
    public IDisposable OnChange(Func<IChangeToken> changeTokenProducer, Func<Task> changeTokenConsumer);
    public IDisposable OnChange<TState>(Func<IChangeToken> changeTokenProducer, Func<TState, Task> changeTokenConsumer, TState state);
}

API Usage

using Microsoft.Extensions.Primitives;

var config = new ConfigurationBuilder().AddJsonFile("config.json", optional: false, reloadOnChange: true)
            .Build();

_ = ChangeToken.OnChange(config.GetReloadToken, async () =>
{
    await Task.Delay(1000);
    Console.WriteLine("Change happened");
});

Alternative Designs

No response

Risks

None

[ghost]

Tagging subscribers to this area: @mangod9 See info in area-owners.md if you want to be subscribed.

Issue Details

---

### Background and motivation Today it's not possible to use `ChangeToken.OnChange` to execute asynchronous logic before re-subscribing for new updates. The current API has a `Action` based callback only (https://docs.microsoft.com/en-us/dotnet/api/microsoft.extensions.primitives.changetoken.onchange?view=dotnet-plat-ext-6.0). We need an async overload to run async logic so people don't end up using async void or blocking when using this helper. ### API Proposal ```csharp namespace Microsoft.Extensions.Primitives; public static class ChangeToken { public IDisposable OnChange(Func changeTokenProducer, Func changeTokenConsumer); public IDisposable OnChange(Func changeTokenProducer, Func changeTokenConsumer, TState state); } ``` ### API Usage ```csharp using Microsoft.Extensions.Primitives; var config = new ConfigurationBuilder().AddJsonFile("config.json", optional: false, reloadOnChange: true) .Build(); _ = ChangeToken.OnChange(config.GetReloadToken, async () => { await Task.Delay(1000); Console.WriteLine("Change happened"); }); ``` ### Alternative Designs _No response_ ### Risks None

Author:	davidfowl
Assignees:	-
Labels:	`api-suggestion`, `area-System.Threading`, `untriaged`
Milestone:	-

[ghost]

Tagging subscribers to this area: @dotnet/area-extensions-primitives See info in area-owners.md if you want to be subscribed.

Issue Details

---

### Background and motivation Today it's not possible to use `ChangeToken.OnChange` to execute asynchronous logic before re-subscribing for new updates. The current API has a `Action` based callback only (https://docs.microsoft.com/en-us/dotnet/api/microsoft.extensions.primitives.changetoken.onchange?view=dotnet-plat-ext-6.0). We need an async overload to run async logic so people don't end up using async void or blocking when using this helper. ### API Proposal ```csharp namespace Microsoft.Extensions.Primitives; public static class ChangeToken { public IDisposable OnChange(Func changeTokenProducer, Func changeTokenConsumer); public IDisposable OnChange(Func changeTokenProducer, Func changeTokenConsumer, TState state); } ``` ### API Usage ```csharp using Microsoft.Extensions.Primitives; var config = new ConfigurationBuilder().AddJsonFile("config.json", optional: false, reloadOnChange: true) .Build(); _ = ChangeToken.OnChange(config.GetReloadToken, async () => { await Task.Delay(1000); Console.WriteLine("Change happened"); }); ``` ### Alternative Designs _No response_ ### Risks None

Author:	davidfowl
Assignees:	-
Labels:	`api-suggestion`, `untriaged`, `area-Extensions-Primitives`
Milestone:	-

[maryamariyan]

Marked for future. @davidfowl is this a critical feature for 7.0?

[davidfowl]

Critical, no but I can take it to API review and do the implementation if needed for .NET 7. It's currently a pit of failure (I filed this because I saw someone use it in a way that would fail).

[lonix1]

This would be highly appreciated!

Another use case. There's double detection of changes to appsettings.json. There are still open issues for that. Those issues and the docs recommend debouncing the detections. But using the change token approach needs an async overload. (I documented the problem here).

[dazinator]

I worked around this a while ago: https://github.com/dazinator/Changify/blob/develop/src/Changify/Extensions/ChangeTokenExtensions.cs

Basically, in order to use ChangeToken.OnChange you need:

• Func<IChangeToken> produces the token.
• `Action`` is called when the token changes.

So I wrote the above extension method for Func<IChangeToken> so you can do this:

Func<IChangeToken> changeTokenProducer = GetOrInjectThis();

await changeTokenProducer.WaitOneAsync();

// token has been signalled do your async work here.
await MyStuff();

// One draw back of this approach is that it's one execution at a time, i.e so to listen for the next signal you have to make this call again or refactor this into a loop so can be used similar to ChangeToken.OnChange style.

 await changeTokenProducer.WaitOneAsync();

[davidfowl]

One other idea is an IAsyncEnumerable approach?

[dazinator]

Here is the implementation I landed on: https://github.com/dazinator/Changify/blob/develop/src/Changify/Extensions/ChangeTokenExtensions.cs#L78

The reason the solution looks like this, to me, is basically because I was already familiar with this WaitOneAsync api - and did not want the cognitive load of trying to make an async equivalent to this: https://github.com/dotnet/runtime/blob/3aaebebb9b766bf15e936f29ab90686de7c21840/src/libraries/Microsoft.Extensions.Primitives/src/ChangeToken.cs#L55 or solving from first principles ;-)

and a test: https://github.com/dazinator/Changify/blob/develop/src/Changify.Tests/ChangeTokenExtensionsTests.cs#L76

Saying that, even though it seems to be functional, I don't like the fact it's creating two TCS's per async callback.

• The reason there is a single long running background task is it made it easy to process each token in a sequence for the duration of the subscription.
• The reason this long runnign outer task doesn't directly await each async callback when a token is signalled, but instead schedules it as an independent task, is that this allows uncaught exceptions (e.g from the async callbacks) to be surfaced as they would do in the synchronous scenario, as shown in the test, whilst keeping the overall subscription alive- i.e the outer task will just continue to schedule the next task for the next token when the isolated task completes. So it was really me trying too keep the exception behaviour the same that lead me down the path of needed another TCS here.

All in all I think this will serve my purposes for now, until either:

• I want to revist from first principles again (i.e without skewing myself to use WaitOneAsync)
• The dotnet team releases something better!
• I (or someone else) creates a different consumer api over the top of change tokens - maybe IAsyncEnumerable based, I could imagine enumerating through an IAsyncEnumerable in a long running background thread for example to process something asynchronously in each iteration in response to a change token firing - basically this would not look like the current ChangeToken.OnChange api.

@davidfowl
one thing which I was curious about is that.. the consumer could unregister from callbacks at any point on another thread for example. Meanwhile an async callback could about to be invoked, or could even be running. It felt to me like it might be nice to supply a CancellationToken into the callback which could be signalled if the consumer unsubscribes. However this isn't part of the api at present. Use case would be, the async task does something resource intensive, and if the consumer is no longer interested in being notified, perhaps thats another way of them indicating that even if a callback is currently running, they wish it to be terminated? I am not sure about this perhaps this a seperate thing that they can workaround via their own mechanisms - passing state and such.

[dazinator]

@davidfowl I am also just geenrally curious as to why this OnChange method was not implemented as an extension method to Func<IChangeToken> to aid discoverability e.g current usage is like:

            var producer = new Func<IChangeToken>(() =>
            {
                return someToken
            });
            ChangeToken.OnChange(producer, () => { });           

No big deal but you have to be aware ChangeToken concrete class exists with this static method to discover it. The Extension method style is obvioulsy similar but has the advantage of removing an explicit argument, and is better for discoverability imho:

            var producer = new Func<IChangeToken>(() =>
            {
                return someToken
            });            
            producer.OnChange(async () => { });

This is why the solution I linked above I have gone with an exension method approach, but want to know if there is some valid reasons against that..

[davidfowl]

We don't usually add extension methods on delegate types.

[dazinator]

Some arguments for introducung an IChangeTokenProducer interface:

• Extenion methods such as OnChange or OnChangeAsync, OnWaitOneAsync etc could be introduced without violating your policy.
• A composite builder could be introduced in future, CompositeChangeToken is already a thing, but no builder api to compose seperate producers (sources) together into a single composite producer is available.
• often producers have to be disposable because they wrap disposable resources for their lifetimes such as file watchers or database connections in order to actively signal tokens. Func<IChangeToken> is nice for composability but not so good for lifetime management

[Neme12]

What would the implementation be? Await every single consumer sequentially? Or Task.WhenAll? I think the latter is a much better option. We don't need to artifically delay the callbacks until some other unrelated previous callback finished executing.

[m17kea]

Is this still on the radar?