dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.19k stars 4.72k forks source link

API proposal: Modern Timer API #31525

Closed davidfowl closed 3 years ago

davidfowl commented 4 years ago

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 😄 .

+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

public class Timer
{
+     public Timer(TimeSpan period);
+     ValueTask<bool> WaitForNextTickAsync(CancellationToken cancellationToken);
}

cc @stephentoub

willdean commented 3 years ago

Does "The timer will be paused while user code is executing and will resume the next period once it ends." mean that the actual cycle period is the total of the code execution time plus the configured timer interval? If so, is there any substantive difference between the proposed timer and simply using Task.Delay(period), like this:

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

        while (true)
        {
            await Task.Delay(second);
            Console.WriteLine($"Tick {DateTime.Now}")
        }
    }
}

But maybe that's not what "paused" implies?

davidfowl commented 3 years ago

I hope in API review we discuss the other option as well (where the next dueTime is interval - time to execute your code) as it could support both easily.

It's also more efficient than Task.Delay:

The last thing is that the pattern that makes it nice is the ValueTask<bool> return type means you don't need to While(true). If we figure out a way to make it IAsyncEnumerable work, then its possible we still end up with that as a pattern.

willdean commented 3 years ago

Thanks @davidfowl, that makes sense. If either is feasible, then I would vote (as if I have a vote!) for the cycle time to be the configured interval (or best effort at) rather than (the interval + the execution time). To me that that's what a 'timer' implies - something which keeps time independently of the code execution duration.

But this would require a definition of what happens about code execution which (occasionally, one presumes) overruns the configured interval, and there are several different ways of looking at this which are all right/wrong in different situations.

I think there are three main possibilities:

  1. It's just a delay, and the actual cycle period is configured interval + execution time.
  2. The cycle period is the configured interval, and if the execution overruns you'll get re-scheduled immediately for the tick which you've missed. Would there be a limit to how many of these late catch-up ticks you get immediately scheduled for?
  3. The cycle period is effectively referenced to the original start time, so if you overrun a tick it's lost and nothing happens until the next tick is due.

(1) is like using Task.Delay and (2) & (3) are both things one can construct using System.Threading.Timer depending on how you handle re-entrant callbacks.

omariom commented 3 years ago

A TimeSpan parameter in WaitForNextTickAsync would make the next scenario possible:

    TimeSpan keepUp = TimeSpan.FromSeconds(1);
    TimeSpan next = keepUp;

    using var timer = new Ticker();

    while (await timer.WaitForNextTickAsync(next, default))
    {
        var sw = Stopwatch.StartNew();

        // ... some work

        next = (keepUp - sw.Elapsed);

        if (next < TimeSpan.Zero)
        {
            next = TimeSpan.Zero;
        }
    }
terrajobst commented 3 years ago

Video

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

@terrajobst, we didn't talk about assembly for this. Where should it live?

terrajobst commented 3 years ago

Good question; I'd start with wherever System.Threading.Timer lives today.

stephentoub commented 3 years ago

System.Threading.Timer is implemented in corelib and exposed from System.Runtime.

terrajobst commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/dotnet/runtime/issues/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 commented 3 years ago

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 commented 3 years ago

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

tebeco commented 3 years ago

Hello,

regarding @benaadams's comment about the list of all existing timers here: https://github.com/dotnet/runtime/issues/31525#issuecomment-794597247

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 commented 3 years ago

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 commented 3 years ago

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

davidfowl commented 3 years ago

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'
omariom commented 3 years ago
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 commented 3 years ago

Instead, it could be

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

omariom commented 3 years ago

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

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

stephentoub commented 3 years ago

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 commented 3 years ago

Do you have an example?

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

stephentoub commented 3 years ago

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 commented 3 years ago

OK this sounds interesting, lets try it.

stephentoub commented 3 years ago

@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.

tebeco commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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