Skip to content
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

API proposal: Modern Timer API #31525

Closed
davidfowl opened this issue Nov 20, 2019 · 80 comments · Fixed by #53899
Closed

API proposal: Modern Timer API #31525

davidfowl opened this issue Nov 20, 2019 · 80 comments · Fixed by #53899
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading
Milestone

Comments

@davidfowl
Copy link
Member

davidfowl commented Nov 20, 2019

To solve of the common issues with timers:

  1. The fact that it always captures the execution context is problematic for certain long lived operations (https://github.com/dotnet/corefx/issues/32866)
  2. The fact that is has strange rooting behavior based on the constructor used
  3. The fact timer callbacks can overlap (https://github.com/dotnet/corefx/issues/39313)
  4. The fact that timer callbacks aren't asynchronous which leads to people writing sync over async code

I propose we create a modern timer API based that basically solves all of these problems 😄 .

  • This API only makes sense for timers that fire repeatedly, timers that fire once could be Task based (we already have Task.Delay for this)
  • The timer will be paused while user code is executing and will resume the next period once it ends.
  • The timer can be stopped using a CancellationToken provided to stop the enumeration.
  • The execution context isn't captured.
+namespace System.Threading
+{
+   public class AsyncTimer : IDisposable, IAsyncDisposable
+   {
+        public AsyncTimer(TimeSpan period);
+        public ValueTask<bool> WaitForNextTickAsync(CancellationToken cancellationToken);
+        public void Stop();
+   }
+}

Usage:

class Program
{
    static async Task Main(string[] args)
    {
        var second = TimeSpan.FromSeconds(1);
        using var timer = new AsyncTimer(second);

        while (await timer.WaitForNextTickAsync())
        {
            Console.WriteLine($"Tick {DateTime.Now}")
        }
    }
}
public class WatchDog
{
    private CanceallationTokenSource _cts = new();
    private Task _timerTask;

    public void Start()
    {
        async Task DoStart()
        {
            try
            {
                await using var timer = new AsyncTimer(TimeSpan.FromSeconds(5));

                while (await timer.WaitForNextTickAsync(_cts.Token))
                {
                    await CheckPingAsync();
                }
            }
            catch (OperationCancelledException)
            {
            }
        }

        _timerTask = DoStart();
    }

    public async Task StopAsync()
    {
        _cts.Cancel();

        await _timerTask;

        _cts.Dispose();
    }
}

Risks

New Timer API, more choice, when to use over Task.Delay?

Alternatives

Alternative 1: IAsyncEnumerable

The issue with IAsyncEnumerable is that we don't have a non-generic version. In this case we don't need to return anything per iteration (the object here is basically void). There were also concerns raised around the fact that IAsyncEnumerable<T> is used for returning data and not so much for an async stream of events that don't have data.

public class Timer
{
+     public IAsyncEnumerable<object> Periodic(TimeSpan period);
}
class Program
{
    static async Task Main(string[] args)
    {
        var second = TimeSpan.FromSeconds(1);

        await foreach(var _ in Timer.Periodic(second))
        {
            Console.WriteLine($"Tick {DateTime.Now}")
        }
    }
}

Alternative 2: add methods to Timer

  • Avoids a new type
  • Confusing to think about what happens when WaitForNextTickAsync is called when a different constructor is called.
public class Timer
{
+     public Timer(TimeSpan period);
+     ValueTask<bool> WaitForNextTickAsync(CancellationToken cancellationToken);
}

cc @stephentoub

@natalie-o-perret
Copy link

natalie-o-perret commented Nov 20, 2019

I like the idea, but when it comes to stop the timer I found it a little bit weird (I mean I guess in your example, that would involve a break to get out of the foreach loop).

@davidfowl
Copy link
Member Author

I like the idea, but when it comes to stop the timer I found it a little bit weird (I mean I guess in your example, that would involve a break to get out of the foreach loop).

Right, it's pull so you stop the timer by breaking out of the loop, or using a cancellation token with the IAsyncEnumerable to stop it from running (that also works if you're not directly in the loop).

@NtFreX
Copy link

NtFreX commented Nov 20, 2019

public static class Timer
{
    public static async Task Subscribe(this IAsyncEnumerable<TimerSpan> ticker, Action<TimerSpan> action)
    {
        await foreach(var t in ticker)
        {
            action?.Invoke(t);
        }
    }
}

var second = TimeSpan.FromSeconds(1);
using(var ticker = Timer.CreateTimer(second, second))
{
    await ticker.Subscribe(t => Console.WriteLine($"Tick {t}"));
}

As long as we get some good helper methods simular to the one above I like fanciness.

@davidfowl
Copy link
Member Author

The entire point of this is to avoid callbacks, but if you want to write that helper, have at it 😄

@NicolasDorier
Copy link

NicolasDorier commented Nov 20, 2019

That is sexy. Though it is difficult to know from the look of it, what happen if a tick happen while we are not awaiting. Do they queue up so every tick will get served? or are they ignored if nobody here to listen to them?

@ankitbko
Copy link
Member

ankitbko commented Nov 20, 2019

public static class Timer
{
    public static async Task Subscribe(this IAsyncEnumerable<TimerSpan> ticker, Action<TimerSpan> action)
    {
        await foreach(var t in ticker)
        {
            action?.Invoke(t);
        }
    }
}

var second = TimeSpan.FromSeconds(1);
using(var ticker = Timer.CreateTimer(second, second))
{
    await ticker.Subscribe(t => Console.WriteLine($"Tick {t}"));
}

As long as we get some good helper methods simular to the one above I like fanciness.

Seems very similar to rx timer. Always felt rx should slowly integrate into coreclr/fx.

@willdean
Copy link
Contributor

willdean commented Nov 20, 2019

With apologies for the bike-shedding, but can I suggest it's called something other than Timer?

We already have these three famous ones:
System.Threading.Timer
System.Timers.Timer
System.Windows.Forms.Timer
Plus another few I didn't know about until I looked at Reference Source just now.

People don't always qualify which one they're talking about (#32866 is a good example), and adding another new one just makes things hard to learn for newcomers and hard to talk about.

If it was called Timer2020 then as long as there wasn't more than one new timer class added each year, that would be a scaleable naming scheme... (Not a serious suggestion, in case my audience is unappreciative.)

@svick
Copy link
Contributor

svick commented Nov 20, 2019

public class Timer

I take it this is System.Threading.Timer, and not System.Timers.Timer, or some new namespace?

public static IAsyncEnumerable<TimerSpan> Create(TimeSpan dueTime, TimeSpan period);

Assuming this method is indeed added to System.Threading.Timer, I feel like calling it Create is not sufficient to differentiate it from the Timer constructor. Though I'm not sure what name would be better here.


In general, I agree that having await-based timer is useful, but I'm not sure IAsyncEnumerable<T> is the right abstraction for that.

For example, consider that I want to output items from a collection to the console, but only at a rate of one per second. With a different design, that could be easily achieved by something like:

using var timer = Timer.Create(TimeSpan.Zero, TimeSpan.FromSeconds(1));

foreach (var item in items)
{
    await timer.WaitForNext();
    
    Console.WriteLine(item);
}

But I think this would be much harder to write using an IAsyncEnumerable<T>-based timer, at least without Ix.NET.

@vshapenko
Copy link

Maybe this should be an extension to System.Threading.Channels?

@davidfowl
Copy link
Member Author

davidfowl commented Nov 20, 2019

@svick you can use IAsyncEnumerable for that as well. What you wrote is literally GetEnumerator + MoveNextAsync

@alefranz
Copy link
Contributor

  • If user code takes longer to run than the interval, we drop the tick events that happened until the user can catch up (think bounded channel with drop and a capacity of 1).

That's definitely the right approach, but I think it would be nice to customize what happens when a tick is missed: return immediately on await vs wait for the next tick (so to get back in sync)

@stephentoub
Copy link
Member

stephentoub commented Nov 20, 2019

Just to make sure I understand the proposal, you're basically proposing this, right? (I'm not sure what the TimeSpan is meant to be an offset from, so I've just used DateTime here.)

public static IAsyncEnumerable<DateTime> Create(TimeSpan dueTime, TimeSpan period)
{
    var c = Channel.CreateBounded<DateTime>(1);
    using (new Timer(s => ((Channel<DateTime>)s).Writer.TryWrite(DateTime.UtcNow), c, dueTime, period))
        await foreach (DateTime dt in c.Reader.ReadAllAsync())
            yield return dt;
}

?

@ankitbko
Copy link
Member

If we are open for new name, I would propose Interval.

But adding something similar to Timer under new name smells bad. Creates unnecessary confusion.

@omariom
Copy link
Contributor

omariom commented Nov 20, 2019

Now we need this

IAsyncEnumerable<Message> channel = channelReader.ReadAllAsync();
IAsyncEnumerable<TimeSpan> ticker = Timer.CreateTimer(second, second);

(channel, ticker) switch 
{
    TimeSpan tick => 
    {
    },
    Message msg => 
    {
    }
}

I can't find @stephentoub's proposal on the subject 😐 but there was definitely one

@davidfowl
Copy link
Member Author

@stephentoub Yep. But with BoundedChannelFullMode.DropOldest, though that really only matters if we find something useful to put in the <T>. Originally I was thinking time since last tick so if the code execution lasts longer than the period, you'd get a timespan that represnted how long between MoveNext calls but it was late, I dunno if that's useful 😄

@stephentoub
Copy link
Member

stephentoub commented Nov 20, 2019

Now we need this

That's problematic with IAsyncEnumerable<T>, assuming you want to perform this operation more than once, because a) you need to get an enumerator in order to read from it and that enumerator may "start over" depending on the source, but more importantly b) once you call MoveNext() you've pushed the enumerator forward and you can't take it back; that effectively ruins it for a subsequent consumer.

With channels it's feasible, because rather than MoveNext/Current, the operations are WaitForReadAsync/TryRead, which means unlike MoveNext, asking to be notified of an available item doesn't consume said item.

For a while we were on a track with IAsyncEnumerable<T> of employing a similar interface pattern, but for such a mainstream interface it brought significant complexities.
https://github.com/dotnet/corefx/issues/32640#issuecomment-428190420

@davidfowl
Copy link
Member Author

That's definitely the right approach, but I think it would be nice to customize what happens when a tick is missed: return immediately on await vs wait for the next tick (so to get back in sync)

Why?

@willdean
Copy link
Contributor

willdean commented Nov 20, 2019

Why?

I can't speak for the OP, nor whether this sort of an option would actually be a good thing, but sometimes I want a one-second timer to mean "run this code 3600 times an hour", and sometimes I want it to mean "wake this code up roughly once a second".

Clearly all such things are no better than 'best effort' on this sort of platform, but to me there is a discernible difference in intention.

@scalablecory
Copy link
Contributor

Reactive's Observable.Interval is nice, but it seems weird usability-wise for the return to be an IAsyncEnumerable.

@davidfowl
Copy link
Member Author

@scalablecory can you elaborate?

@scalablecory
Copy link
Contributor

@davidfowl It's the IAsyncEnumerable use. Something like this seems a better way to accomplish the same thing:

var timer = new Timer(delay);

while(await timer.WaitNextPeriodAsync())
{
}

If you wouldn't ever .ToArrayAsync() on it, then it doesn't fit IAsyncEnumerable. I can't imagine ever passing this timer enumerable to anything else (like LINQ) that operates on an IAsyncEnumerable -- if I were ever doing that, Rx would probably be a better choice for such an event-based architecture. foreaching a timer also seems weird.

@scalablecory
Copy link
Contributor

I'd also want us to look at solving the problems you called out in the existing API as well. It is very common to need an interval timer but not have a neat sequence point to await at -- instead, callbacks that happen in parallel to your bigger machine are exactly what you need.

@davidfowl
Copy link
Member Author

davidfowl commented Nov 21, 2019

I disagree with the assertion that foreaching this is weird. The difference between Rx and Ix is push vs pull, nothing else. In fact the pattern you show is just a GetAsyncEnumerator().MoveNextAsync().

Also looking around, golang has a very similar feature called a Ticker that exposes a channel of ticks. It’s a pretty elegant pattern IMHO 😬. I’m it sure why ToArray matters (IEnumerable can also be infinite)

I'd also want us to look at solving the problems you called out in the existing API as well. It is very common to need an interval timer but not have a neat sequence point to await at -- instead, callbacks that happen in parallel to your bigger machine are exactly what you need.

Sure we can do that at the cost of adding more complexity to the existing API (like passing more options to the existing timer ctor?). That’s come up in the past as a potential problem.

@svick
Copy link
Contributor

svick commented Nov 21, 2019

@davidfowl

In fact the pattern you show is just a GetAsyncEnumerator().MoveNextAsync().

I think the problem is that when you have an IEnumerable<T> or IAsyncEnumerable<T>, you're basically saying "you should use foreach". If it's expected that you're commonly going to be calling MoveNextAsync manually, then it's probably better to be explicit about it by having something like WaitNextPeriodAsync instead.

@benaadams
Copy link
Member

you're basically saying "you should use foreach".

Generalising more its an event pattern; which you couldn't do previously with synchronous enumerators, but now can with asyncenumerators:

await foreach (var event in myEventSource)
{

}

@davidfowl
Copy link
Member Author

I think the problem is that when you have an IEnumerable or IAsyncEnumerable, you're basically saying "you should use foreach". If it's expected that you're commonly going to be calling MoveNextAsync manually, then it's probably better to be explicit about it by having something like WaitNextPeriodAsync instead.

Generalising more its an event pattern; which you couldn't do previously with synchronous enumerators, but now can with asyncenumerators:

This is exactly the point. You should use foreach. IAsyncEnumerable + await foreach is a new paradigm for C#.

@svick
Copy link
Contributor

svick commented Nov 21, 2019

@davidfowl In my opinion, it's a paradigm that does not fit well here, so it shouldn't be used.

I think with IAsyncEnumerable<T>, the important part is the data and the fact that you sometimes have to wait for the next item is mostly incidental. But here, the waiting is what you want and you can invent some data to go along with it, but it's completely incidental.

This can be also seen in the syntax: await foreach emphasizes the data and hides the mechanism of retrieving it (await MoveNextAsync()), but that's the opposite of what you want here: you don't care about the data, which is visible, but you do care about the await MoveNextAsync(), which is hidden.

One more way this surfaces is in how you would document this API. For WaitNextPeriodAsync(), it would be roughly:

Here's this one method, await it when you want to wait for the next timer tick. Surround it with whatever syntax you want.

For IAsyncEnumerable<T>, it would be (exaggerated for effect):

It's a sequence of … timestamps, I guess? Though it doesn't behave like most other sequences, since it changes depending on how quickly you iterate it. And maybe iterating it using await foreach, like you normally would, doesn't fit your use case, in which case you should await MoveNextAsync yourself.

To sum up, I think that IAsyncEnumerable<T> doesn't fit well logically, its syntax focuses on the wrong thing and it makes it harder to explain what the behavior of the API is and how you should use it. Which is why it shouldn't be used here.

@benaadams
Copy link
Member

Don't think DateTime/TimeSpan have the precision for this; but another example which WaitNextPeriodAsync() by itself doesn't capture (why do you want to tick without knowing the time?)

const int framesPerSecond = 60;

var desiredFrameRate = TimeSpan.FromSeconds(1.0 / framesPerSecond);
var ticker = Timer.CreateTimer(desiredFrameRate, desiredFrameRate);

var current = DateTime.UtcNow;
await foreach(var time in ticker)
{
    var delta = time - current;
    current = time;
    
    Console.WriteLine($"Elasped: {delta}, fps: {(double)TimeSpan.TicksPerSecond / delta.Ticks:N2}");
}

However, I don't think the data types have the precision for the above sample as

var current = DateTime.UtcNow;
var time = current + TimeSpan.FromSeconds(1.0 / 60);

var delta = time - current;

Outputs:

Elasped: 00:00:00.0170000, fps: 58.82

Rather than

Elasped: 00:00:00.0166667, fps: 60.00

@scalablecory
Copy link
Contributor

scalablecory commented Nov 21, 2019

@benaadams That use seems problematic, though, doesn't it?

For render lerping it would be more useful to get the time inside the loop (after your async continuation has started running) rather than have time be the moment the timer scheduled the continuation. QueryPerformanceCounter/etc.

For physics & entity ticks it would be more useful to have fixed intervals, not dynamic.

You would also not want it to skip overlapping intervals as the issue desires as that'd halve your framerate, though we could always add a flag to control that feature.

However, I don't think the data types have the precision for the above

TimeSpan has precision down to 1e-7 seconds; I assume you're seeing scheduling delay?

@scalablecory
Copy link
Contributor

scalablecory commented Nov 21, 2019

I disagree with the assertion that foreaching this is weird.

I think @benaadams' example convinced me that it's not entirely bad, though I still think it's weird.

The difference between Rx and Ix is push vs pull, nothing else.

I think that's an over-simplification by looking only at their abstraction interfaces, not at how they're currently used or the frameworks built around them -- there's a very large difference between the two. Rx is a rich event-driven programming model, whereas enumerables are currently only used for iterating data.

In fact the pattern you show is just a GetAsyncEnumerator().MoveNextAsync().

Indeed. It's not the basic idea I'm concerned with, but rather specifically the use of IAsyncEnumerable.

Right now I feel like enumerables are very data-focused, as @svick said (more eloquently than me). What other APIs in corefx do we have as an example of using enumerables for non-data work? Doing something new isn't a bad thing, I just want to make sure it's clear if we're doing something new as it would set the direction for our user's coding conventions. Enumerables currently have a very predictable usage and I'm really against polluting that by having to think about if an enumerable is there for scheduling or for data

I know that Unity uses IEnumerable with yield return for ticking entity state, so there is some prior work here at least outside of corefx.

@terrajobst
Copy link
Member

terrajobst commented Mar 9, 2021

Seems like the right place for a threading primitive. Any objections to that place? We can of course put it in a higher layer and type forward down if needed, but we know that changes like this are painful...

@benaadams
Copy link
Member

Any objections to that place?

I thought Timers were a special type where each one needed to go in a different namespace? 😉

image

More seriously System.Threading sounds like a good place.

@danmoseley
Copy link
Member

We maybe should improve that article to help people pick the right one., When looking for it, I actually found an article from 2004, when there were three, even back then it was apparently worth a magazine article to help explain "The .NET Framework Class Library provides three different timer classes..."

@lloydjatkinson
Copy link

lloydjatkinson commented Mar 10, 2021

How do we write unit tests that use this? Has any thought been given to testability?

For reference: Jest Timer Mocks. Would functionality like this “run all timers” be available for a unit test runner like xunit to implement?

@sander1095
Copy link

Perhaps this has already been discussed; I can't easily find this out: How does this deal with a very long time between executions (couple of weeks)?

Sometime ago I was looking into a way to run a background service on a timer with a cronjob. Perhaps an idea for #36383 👀 ...

I found this article: https://codeburst.io/schedule-cron-jobs-using-hostedservice-in-asp-net-core-e17c47ba06

Basically, they do something like:

_timer = new System.Timers.Timer(delay.TotalMilliseconds);

but if the delay is too big, it won't fit in the double..

Will this API work for this?

@weltkante
Copy link
Contributor

weltkante commented Mar 10, 2021

but if the delay is too big, it won't fit in the double

Any delay (in milliseconds) should fit into a double, you have 53 bits (or 15 decimal digits) precision, thats more than two hundred thousand years worth of waiting for your timeout if I didn't miscalculate. Thats longer than the machine is going to be running, considering the timers aren't persistent (will not survive restart of the process) double precision should be sufficient for any practical purpose.

But why sacrifice API quality and not just pass a TimeSpan directly?

@stephentoub
Copy link
Member

Timer supports TimeSpans up to ~45 days, which means PeriodTimer would support such periods as well. Larger values will fail.

@tebeco
Copy link

tebeco commented Apr 13, 2021

Hello,

regarding @benaadams's comment about the list of all existing timers here:
#31525 (comment)

Some of them should be concidered Obsolete to use when targeting .Net6 ?
The question about Timers is redundant on Community such as the CSharp Discord or Slack.
The confusion is especially affected by the two System.Timers.Timer vs System.Threading.Timer.
So my question is (naive on purpose), can something be done or are they both fully legitimate in modern code base ?

For example on the page of the WebClient we can see this:
image

Even with that we see about 1 question every month using the WebClient, and not for upload, just for simple GET and deserialization.
They just did not new HttpClient existed. Other were hinted by teachers about WebClient even if they were free to use HttpClient (not a forced choice I mean)

If all class are legitimate, then my question would turn to the docs and wonder if there's like a "decision graph"
"do you use netfx ..." "do you use ....."

All of this of course assuming the PeriodicTimer being merged in .Net6, so also part of that big equation

@davidfowl
Copy link
Member Author

Fun fact, node just stabilized their awaitable timer APIs: https://nodejs.org/api/timers.html#timers_timers_promises_api. They went with an async stream (async enumerable pattern):

import {
  setInterval,
} from 'timers/promises';

const interval = 100;
for await (const startTime of setInterval(interval, Date.now())) {
  const now = Date.now();
  console.log(now);
  if ((now - startTime) > 1000)
    break;
}
console.log(Date.now());

@stephentoub
Copy link
Member

Does that change your opinion on what shape you prefer here?

@davidfowl
Copy link
Member Author

davidfowl commented Apr 22, 2021

I'm not sure, I think it has similar problems to what we initially discussed in API review. Seems like you can pass a value that always gets returned when for the enumeration which I find bizarre:

for await (const startTime of setInterval(1000,'1')) {
    console.log(startTime);
}

This just prints 1 over and over...

The only way to stop it from the outside is to use the equivalent of a cancellation token (the abort signal):

var controller = new AbortController();

setTimeout(() => {
    controller.abort();
}, 
5000);

for await (const startTime of setInterval(1000,'1', { signal: controller.signal })) {
    console.log(startTime);
}
1
1
1
1
node:timers/promises:152
          callback(PromiseReject(new AbortError()));
                                 ^

AbortError: The operation was aborted
    at EventTarget.onCancel (node:timers/promises:152:34)
    at EventTarget.[nodejs.internal.kHybridDispatch] (node:internal/event_target:461:20)
    at EventTarget.dispatchEvent (node:internal/event_target:409:26)
    at abortSignal (node:internal/abort_controller:98:10)
    at AbortController.abort (node:internal/abort_controller:123:5)
    at Timeout._onTimeout (C:\dev\git\nodetest\app.js:11:20)
    at listOnTimeout (node:internal/timers:557:17)
    at processTimers (node:internal/timers:500:7) {
  code: 'ABORT_ERR'

@stephentoub stephentoub self-assigned this Jun 8, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 8, 2021
@omariom
Copy link
Contributor

omariom commented Jun 10, 2021

namespace System.Threading
{
   public class PeriodicTimer : IDisposable
   {
        public PeriodicTimer(TimeSpan period);
        public ValueTask<bool> WaitForNextTickAsync(CancellationToken cancellationToken = default);
        public void Stop();
   }
}

In such form the usage won't be very different from a while loop with a Task.Delay(..) within. It is not .. innovative.
Less allocations is not obvious, one would need to read about it in the remarks section of the docs. Also Task.Delays are easier to understand.

Instead, it could be

   public class PeriodicTimer : IAsyncEnumerable<..>, IDisposable
   {
        public PeriodicTimer(TimeSpan period);
        public void Stop();
        ...
   }

@stephentoub
Copy link
Member

Instead, it could be

Every call to GetAsyncEnumerator should really produce its own isolated state... what would Stop here do?

@omariom
Copy link
Contributor

omariom commented Jun 10, 2021

Every call to GetAsyncEnumerator should really produce its own isolated state.

Does it have to?
Sometimes collections return this in GetEnumerator.

@stephentoub
Copy link
Member

Does it have to?

Yes

Sometimes collections return this in GetEnumerator.

Do you have an example? That's typically only done as an optimization for the first enumeration.

@omariom
Copy link
Contributor

omariom commented Jun 11, 2021

Do you have an example?

Hmm.. haven't found a single one. That was probably in my collections :/

@stephentoub
Copy link
Member

stephentoub commented Jun 21, 2021

Maybe we take @omariom's suggestion of (I made the T==TimeSpan, and I replaced Stop with Dispose):

   public class PeriodicTimer : IAsyncEnumerable<TimeSpan>, IDisposable
   {
        public PeriodicTimer(TimeSpan period);
        public void Dispose();
        public IAsyncEnumerator<TimeSpan> GetAsyncEnumerator(); // yielded values are time since GetAsyncEnumerator called
   }

and say that any number of consumers can call GetAsyncEnumerator, each gets their own state (including their own underlying timer, etc.), the timer isn't started until GetAsyncEnumerator is called, and Stop stops all of them.

@davidfowl, thoughts?

@davidfowl
Copy link
Member Author

OK this sounds interesting, lets try it.

@stephentoub
Copy link
Member

@davidfowl and I spoke. Two tweaks to the above:

  • Functionally, we allow GetAsyncEnumerator to only ever be called once on the same instance. Subsequent calls get an InvalidOperationException. We can relax that in the future should we find a real need to enumerate the same instance multiple times.
  • We remove the constructor and instead make this creatable from Timer, with a static CreatePeriodic method. System.Threading.Timer remains the main place you go for this stuff.
namespace System.Threading
{
    public sealed class Timer
    {
        public static PeriodicTimer CreatePeriodic(TimeSpan period);
    }

    public sealed class PeriodicTimer : IAsyncEnumerable<TimeSpan>
    {
        public IAsyncEnumerator<TimeSpan> GetAsyncEnumerator(CancellationToken cancellationToken);
        public void Dispose();
    }
}

This means that if you just want to foreach, it's:

await foreach (TimeSpan ts in Timer.CreatePeriodic(period))
{
    ...
}

If you want to enumerate with cancellation, it's:

await foreach (TimeSpan ts in Timer.CreatePeriodic(period).WithCancellation(cancellationToken))
{
    ...
}

If you want to enumerate while also enabling another thread to stop you, it's:

private PeriodicTimer _timer = Timer.CreatePeriodic(period);
...
// Thread 1
await foreach (TimeSpan ts in _timer)
{
    ...
}
...
// Thread 2, at some point
_timer.Dispose();

And, of course, since this is an async enumerable, you can consume it with something like async LINQ, if desired.

@tebeco
Copy link

tebeco commented Jun 22, 2021

naive question on the last example:

private PeriodicTimer _timer = Timer.CreatePeriodic(period);
...
// Thread 1
await foreach (TimeSpan ts in _timer)
{
    await FooAsync(how do i get the cancellation here ?);

    // is there ?
    await FooAsync(_timer.CancellationToken);
}

// or is it with a tupple like PipeReader api
await foreach ( var (ts, ct) in timer))
{
  await FooAsync(ct);
}
...
// Thread 2, at some point
_timer.Dispose();

if the thread2 cancel or dispose the timer
how do i make the cancellation flow down ?

@stephentoub
Copy link
Member

if the thread2 cancel or dispose the timer how do i make the cancellation flow down ?

Disposing the timer will cause MoveNextAsync to start returning false. Canceling the cancellation token will cause MoveNextAsync to throw a cancellation exception.

@tebeco
Copy link

tebeco commented Jun 23, 2021

Disposing the timer will cause MoveNextAsync to start returning false. Canceling the cancellation token will cause MoveNextAsync to throw a cancellation exception.

that works for the next tick, i'm wondering about the code inside the loop

private PeriodicTimer _timer = Timer.CreatePeriodic(period);
...
// Thread 1
await foreach (TimeSpan ts in _timer)
{
    // how do you cancel the line below "ASAP"
    // you don't always want to wait for MoveNextAsync
    await FooAsync(); // <== long operation
}
...
// Thread 2, at some point
_timer.Dispose();

if you want the outer+inner code to be cancellable at once you need to cancel both the loop and the code inside

i understand that there's way

  • passing the cancellation-token into the thread state object
  • sharing by closure
  • etc ...

Since we're explicitly iteration on the timer i though we would have that token hold by the timer

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 24, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading
Projects
None yet
Development

Successfully merging a pull request may close this issue.