-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Comments
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 |
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). |
As long as we get some good helper methods simular to the one above I like fanciness. |
The entire point of this is to avoid callbacks, but if you want to write that helper, have at it 😄 |
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? |
Seems very similar to rx timer. Always felt rx should slowly integrate into coreclr/fx. |
With apologies for the bike-shedding, but can I suggest it's called something other than We already have these three famous ones: 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.) |
public class Timer I take it this is public static IAsyncEnumerable<TimerSpan> Create(TimeSpan dueTime, TimeSpan period); Assuming this method is indeed added to In general, I agree that having 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 |
Maybe this should be an extension to System.Threading.Channels? |
@svick you can use IAsyncEnumerable for that as well. What you wrote is literally GetEnumerator + MoveNextAsync |
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) |
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;
} ? |
If we are open for new name, I would propose But adding something similar to Timer under new name smells bad. Creates unnecessary confusion. |
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 |
@stephentoub Yep. But with |
That's problematic with 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 |
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. |
Reactive's |
@scalablecory can you elaborate? |
@davidfowl It's the var timer = new Timer(delay);
while(await timer.WaitNextPeriodAsync())
{
} If you wouldn't ever |
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 |
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)
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. |
I think the problem is that when you have an |
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)
{
} |
This is exactly the point. You should use foreach. IAsyncEnumerable + await foreach is a new paradigm for C#. |
@davidfowl In my opinion, it's a paradigm that does not fit well here, so it shouldn't be used. I think with This can be also seen in the syntax: One more way this surfaces is in how you would document this API. For
For
To sum up, I think that |
Don't think DateTime/TimeSpan have the precision for this; but another example which 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:
Rather than
|
@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 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.
|
I think @benaadams' example convinced me that it's not entirely bad, though I still think it's weird.
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.
Indeed. It's not the basic idea I'm concerned with, but rather specifically the use of 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 |
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... |
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..." |
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? |
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:
but if the delay is too big, it won't fit in the Will this API work for this? |
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? |
Timer supports TimeSpans up to ~45 days, which means PeriodTimer would support such periods as well. Larger values will fail. |
Hello, regarding @benaadams's comment about the list of all existing timers here: Some of them should be concidered Obsolete to use when targeting .Net6 ? For example on the page of the Even with that we see about 1 question every month using the If all class are legitimate, then my question would turn to the docs and wonder if there's like a "decision graph" All of this of course assuming the |
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()); |
Does that change your opinion on what shape you prefer here? |
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);
}
|
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. Instead, it could be public class PeriodicTimer : IAsyncEnumerable<..>, IDisposable
{
public PeriodicTimer(TimeSpan period);
public void Stop();
...
} |
Every call to GetAsyncEnumerator should really produce its own isolated state... what would Stop here do? |
Does it have to? |
Yes
Do you have an example? That's typically only done as an optimization for the first enumeration. |
Hmm.. haven't found a single one. That was probably in my collections :/ |
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? |
OK this sounds interesting, lets try it. |
@davidfowl and I spoke. Two tweaks to the above:
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. |
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 |
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
Since we're explicitly iteration on the timer i though we would have that token hold by the timer |
To solve of the common issues with timers:
I propose we create a modern timer API based that basically solves all of these problems 😄 .
Task.Delay
for this)Usage:
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); }
Alternative 2: add methods to Timer
cc @stephentoub
The text was updated successfully, but these errors were encountered: