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]: Expose method on PeriodicTimer to adjust interval #60384

Closed
andrewmd5 opened this issue Oct 14, 2021 · 19 comments · Fixed by #82560
Closed

[API Proposal]: Expose method on PeriodicTimer to adjust interval #60384

andrewmd5 opened this issue Oct 14, 2021 · 19 comments · Fixed by #82560
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading
Milestone

Comments

@andrewmd5
Copy link

andrewmd5 commented Oct 14, 2021

EDITED by @stephentoub on 9/1/2022 to update proposal

namespace System.Threading
{
    public sealed class PeriodicTimer : System.IDisposable
    {
         public PeriodicTimer(System.TimeSpan period) { }
         public System.Threading.Tasks.ValueTask<bool> WaitForNextTickAsync(System.Threading.CancellationToken cancellationToken = default) { throw null; }
         public void Dispose() { }
+	 public TimeSpan Period { get; set; }
    }
}

Background and motivation

After discovering the new PeriodicTimer class that was introduced in .NET 6 I tried to use it in a very simple implementation.

If the period between ticks is a constant interval the class is fine. However, if you have a sliding window where the period between ticks might increase (such is the case with something like an exponential backoff) you are forced to move back to using Task.Delay

To provide a real-world example of this, lets say I have a remote service where a client request a temporary lease to gain access to that service. The lease contains a TTL, and it is up to the client to renew the lease before that TTL comes to pass. Upon renewal, the service returns a new TTL which may be longer or shorter depending on the clients reputation.

It isn't possible today to use PeriodicTimer to create a background task that handles that renewal logic without explicitly creating a new instance or using gotos (implicit):

// rate based on the TTL the remote service gave us.
using var timer = new PeriodicTimer(RenewalTickRate);
while (await timer.WaitForNextTickAsync(_cancellationToken)) {
 var response = await client.RenewLease(...);
 Console.WriteLine($"New Lease TTL: {response.Ttl}");
 // no way to adjust the timer to tick based on the new ttl
}

You can accomplish this with Task.Delay by updating variables outside of a while loop, and I'm sure that is a pattern you can find in thousands of C# codebases, but then you lose out on the performance benefits PeriodicTimer offers. That being said, the underlying TimerQueueTimer that an instance of PeriodicTimer uses already supports changing its state, so I hope this isn't too major of a change to consider.

API Proposal

namespace System.Threading
{
    public sealed class PeriodicTimer : System.IDisposable
    {
         public PeriodicTimer(System.TimeSpan period) { }
         public System.Threading.Tasks.ValueTask<bool> WaitForNextTickAsync(System.Threading.CancellationToken cancellationToken = default) { throw null; }
         public void Dispose() { }
+	 public void Change(TimeSpan period);
    }
}

API Usage

// rate based on the TTL the remote service gave us.
using var timer = new PeriodicTimer(RenewalTickRate);
while (await timer.WaitForNextTickAsync(_cancellationToken)) {
 var response = await client.RenewLease(...);
 timer.Change(response.Ttl);
}

Risks

None I can immediately think of.

@andrewmd5 andrewmd5 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 14, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Oct 14, 2021
@ghost
Copy link

ghost commented Oct 14, 2021

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

Issue Details

Background and motivation

After discovering the new PeriodicTimer class that was introduced in .NET 6 I tried to use it in a very simple implementation.

If the period between ticks is a constant interval the class is fine. However, if you have a sliding window where the period between ticks might increase (such is the case with something like an exponential backoff) you are forced to move back to using Task.Delay

To provide a real-world example of this, lets say I have a remote service where a client request a temporary lease to gain access to that service. The lease contains a TTL, and it is up to the client to renew the lease before that TTL comes to pass. Upon renewal, the service returns a new TTL which may be longer or shorter depending on the clients reputation.

It isn't possible today to use PeriodicTimer to create a background task that handles that renewal logic without explicitly creating a new instance or using gotos (implicit):

// rate based on the TTL the remote service gave us.
using var timer = new PeriodicTimer(RenewalTickRate);
while (await timer.WaitForNextTickAsync(_cancellationToken)) {
 var response = await client.RenewLease(...);
 Console.WriteLine($"New Lease TTL: {response.Ttl}");
 // no way to adjust the timer to tick based on the new ttl
}

You can accomplish this with Task.Delay by updating variables outside of a while loop, and I'm sure that is a pattern you can find in thousands of C# codebases, but then you lose out on the performance benefits PeriodicTimer offers. That being said, the underlying TimerQueueTimer that an instance of PeriodicTimer uses already supports changing its state, so I hope this isn't too major of a change to consider.

API Proposal

namespace System.Threading
{
    public sealed class PeriodicTimer : System.IDisposable
    {
         public PeriodicTimer(System.TimeSpan period) { }
         public System.Threading.Tasks.ValueTask<bool> WaitForNextTickAsync(System.Threading.CancellationToken cancellationToken = default) { throw null; }
         public void Dispose() { }
+	 public void Change(TimeSpan period);
    }
}

API Usage

// rate based on the TTL the remote service gave us.
using var timer = new PeriodicTimer(RenewalTickRate);
while (await timer.WaitForNextTickAsync(_cancellationToken)) {
 var response = await client.RenewLease(...);
 timer.Change(response.Ttl);
}

Risks

None I can immediately think of.

Author: AndrewMD5
Assignees: -
Labels:

api-suggestion, area-System.Threading, untriaged

Milestone: -

@danmoseley
Copy link
Member

danmoseley commented Feb 12, 2022

@stephentoub looks like you added this originally - #53899 - and @davidfowl you proposed it. Any concerns with this API proposal - could I mark as ready for review? (talked to @andrewmd5 about it offline)

@stephentoub
Copy link
Member

Any concerns with this API proposal

The concept seems fine. There are a variety of ways it could be surfaced, e.g. we might consider instead a get/set property.

@danmoseley danmoseley added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Feb 15, 2022
@davidfowl
Copy link
Member

Yep. This looks fine, we can go to api review.

@John0King
Copy link

why not a TimeSpan property ? so we can also get the period time

@ilmax
Copy link

ilmax commented Feb 15, 2022

Was thinking that it may be a nice addition to be able to create a periodic timer given a cron expression. I know that (AFAIK) there's no built in support for cron as of now, but I'm dropping this here to see what other people thinks of it

@stephentoub
Copy link
Member

why not a TimeSpan property ? so we can also get the period time

This is what I meant by "There are a variety of ways it could be surfaced, e.g. we might consider instead a get/set property."

Was thinking that it may be a nice addition to be able to create a periodic timer given a cron expression

That is a very different and much more complicated feature. Please open a separate issue if it's something you're interested in. Thanks.

@Joe4evr
Copy link
Contributor

Joe4evr commented Feb 15, 2022

This is what I meant by "There are a variety of ways it could be surfaced, e.g. we might consider instead a get/set property."

🍝 One possible advantage of the property approach would be making it easy to change the period by a relative amount of time (eg. += TimeSpan.FromMinutes(5);).

@stephentoub stephentoub removed the untriaged New issue has not been triaged by the area owner label Mar 15, 2022
@mangod9 mangod9 added this to the Future milestone Jul 6, 2022
@angelru
Copy link

angelru commented Jan 25, 2023

Any progress on this proposal?

@stephentoub
Copy link
Member

Any progress on this proposal?

It's in the queue to be reviewed.

@stephentoub stephentoub modified the milestones: Future, 8.0.0 Jan 25, 2023
@theodorzoulias
Copy link
Contributor

theodorzoulias commented Jan 28, 2023

What will happen if the Period is changed while a WaitForNextTickAsync operation is currently active?

  1. The ValueTask<bool> will complete immediately with the result false?
  2. The ValueTask<bool> will complete after a shortened or prolonged timespan, according to the delta of the changed Period?
  3. The ValueTask<bool> will be totally unaffected from the change, and the changed Period will only affect the next WaitForNextTickAsync?

@stephentoub
Copy link
Member

What will happen if the Period is changed while a WaitForNextTickAsync operation is currently active?

It'd be "(2) The ValueTask will complete after a shortened or prolonged timespan, according to the delta of the changed Period", noting that there's of course a race condition between the timer firing in response to the current period and the new period taking effect.

@theodorzoulias
Copy link
Contributor

Yep, makes sense. What about the case that the Period is changed after the PeriodicTimer is disposed of? Will it be a no-op, or an ObjectDisposedException will be thrown?

@stephentoub
Copy link
Member

Presumably ODE. Why are you asking?

@theodorzoulias
Copy link
Contributor

Because it is a useful API that I could make use of. Clarifying these details might make the process of approval smoother. :-)

@terrajobst
Copy link
Member

terrajobst commented Feb 23, 2023

Video

  • Looks good as proposed
namespace System.Threading;

public partial class PeriodicTimer
{
    public TimeSpan Period { get; set; }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 23, 2023
@stephentoub stephentoub self-assigned this Feb 23, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 23, 2023
@flimtix
Copy link

flimtix commented Feb 27, 2023

@stephentoub Will this property be exposed in .NET 8 or is there a possibility to use this already in .NET 7?

@stephentoub
Copy link
Member

stephentoub commented Feb 27, 2023

Only .NET 8 and beyond.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 2, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Apr 1, 2023
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.