Non-reentrant grain timers

[ReubenBond]

Fixes #2574

This builds on PRs which need to be merged first.

This PR currently adds a new bool reentrant parameter to RegisterTimer:

protected IGrainTimer RegisterTimer(
    Func<object?, Task> asyncCallback,
    object? state,
    TimeSpan dueTime,
    TimeSpan period,
    bool reentrant = true) // <-- new

The parameter defaults to true to preserves the current behavior. We should not merge it as-is, instead we should decide how we want to expose non-reentrant timers. We could use a different API altogether. At the very least, we should use an overload to retain binary compatibility.

Microsoft Reviewers: Open in CodeFlow

[Rohansi]

It's been a while since I've used Orleans but I think the current timer semantics are to only start counting for the next interval after the current callback completes. I'm not familiar with the internals here but that should definitely be maintained for non-reentrant timers. I saw OneWay mentioned as a possible solution which may affect that so just wanted to mention it.

[rkargMsft]

A new method RegisterGrainTimer could be an option for exposing non-reentrant as an option as it's like the existing Timer, but more Grain-y, and expose options for the new and existing behavior. The existing RegisterTimer can remain and remain with the old behavior. Maybe add an analyzer that upgrades to the new RegisterGrainTimer method before maybe obsoleting the old one.

That could help make it more clear that this has the ability to be more Grain-integrated (ex. non-reentrant by default, lives for the life of the activation only) than a "normal" .NET timer.

[ReubenBond]

An alternative is to extract the parameters out into an "options" object like this:

/// <summary>
/// Functionality for managing grain timers.
/// </summary>
public interface ITimerRegistry
{
// Existing API:
    IGrainTimer RegisterTimer(IGrainContext grainContext, Func<object?, Task> callback, object? state, TimeSpan dueTime, TimeSpan period);
    
// New API - generic, accepts timer options struct
    IGrainTimer RegisterGrainTimer<T>(IGrainContext grainContext, Func<T, Task> callback, T state, TimerCreationOptions options);
}

/// <summary>
/// Options for registering grain timers.
/// </summary>
public readonly struct TimerCreationOptions()
{
    /// <summary>
    /// The amount of time to delay before the timer callback is invoked.
    /// Specify <see cref="Timeout.InfiniteTimeSpan"/> to prevent the timer from starting.
    /// Specify <see cref="TimeSpan.Zero"/> to invoke the callback promptly.
    /// </summary>
    public required TimeSpan DueTime { get; init; }

    /// <summary>
    /// The time interval between invocations of callback.
    /// Specify <see cref="Timeout.InfiniteTimeSpan"/> to disable periodic signaling.
    /// </summary>
    public TimeSpan Period { get; init; } = Timeout.InfiniteTimeSpan;

    /// <summary>
    /// Gets a value indicating whether callbacks scheduled by this timer are allowed to interleave execution with other timers and grain calls.
    /// Defaults to <see langword="false"/>.
    /// </summary>
    /// <remarks>
    /// If this value is <see langword="false"/>, the timer callback will be treated akin to a grain call. If the grain scheduling this timer is reentrant
    /// (i.e., it has the <see cref="ReentrantAttribute"/> attributed applied to its implementation class), the timer callback will be allowed
    /// to interleave with other grain calls and timers regardless of the value of this property.
    /// If this value is <see langword="true"/>, the timer callback will be allowed to interleave with other timers and grain calls.
    /// </remarks>
    public bool Interleave { get; init; }

    /// <summary>
    /// Gets a value indicating whether callbacks scheduled by this timer should extend the lifetime of the grain activation.
    /// Defaults to <see langword="false"/>.
    /// </summary>
    /// <remarks>
    /// If this value is <see langword="false"/>, timer callbacks will not extend a grain activation's lifetime.
    /// If a grain is only processing timer callbacks and no other messages, the grain will be collected after its idle collection period expires.
    /// If this value is <see langword="true"/>, timer callback will extend a grain activation's lifetime.
    /// If the timer period is shorter than the grain's idle collection period, the grain will not be collected due to idleness.
    /// </remarks>
    public bool KeepAlive { get; init; }
}

[ReubenBond]

Added a KeepAlive option, since we don't want to change the grain timer's default semantics. Set it to true to make timers keep grains alive. This opens the path to implement sending requests which do not activate a grain or extend its lifetime (that needs a proposal on the API + semantics first)

[benjaminpetit]

An alternative is to extract the parameters out into an "options" object like this: [...]

I like that. We can still provide the "old" api for a while so people can migrate, or create their own extension to keep compatibility.

I also like @rkargMsft suggestion to call the new api RegisterGrainTimer, to clearly emphasis that it's not a traditional timer.

[ReubenBond]

I like that. We can still provide the "old" api for a while so people can migrate or create their own extension to keep compatibility.

@benjaminpetit in regard to maintaining compat, do you also think we should retain the existing implementation?

I also like @rkargMsft suggestion to call the new api RegisterGrainTimer, to clearly emphasis that it's not a traditional timer.

Done, I've updated the API proposal snippet above. Do you feel we should also rename the options arg type, TimerCreationOptions, to something like GrainTimerCreationOptions?

[ElanHasson]

I like this approach!

[ReubenBond]

Renamed parameter & obsoleted API, otherwise kept everything the same.

[ReubenBond]

New RegisterGrainTimer API. Scheduling information is all in 'options' param.

[ReubenBond]

We never forward local-only requests. We could rename this to IsActivationSpecific or similar if "local-only" isn't clear enough