orleans icon indicating copy to clipboard operation
orleans copied to clipboard

Provide a [Timeout(TimeSpan)] attribute to specify timeouts for grain method calls

Open bill-poole opened this issue 2 years ago • 1 comments

I propose a [Timeout(TimeSpan)] attribute that can be applied to a grain method to specify the timeout for that method call, overriding the default global timeout configured by the SiloMessagingOptions.ResponseTimeout and ClientMessagingOptions.ResponseTimeout options.

An infinite timeout could also be specified by decorating a method with [Timeout(TimeSpan.InfiniteTimeSpan)]. This could be used in place of the [LongRunning] attribute proposed in #7649.

This may also be relevant to #4328.

bill-poole avatar Jul 06 '22 18:07 bill-poole

We're moving this issue to the 4.0-Planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.

ghost avatar Jul 07 '22 18:07 ghost

I really like this idea, and it would be a huge help to our team.

In theory could something like this also work for the ReceiveReminder handler? Right now we have to essentially implement a polling loop to monitor a longer running task that is kicked off by a reminder, which is not ideal.

htxryan avatar Dec 09 '22 11:12 htxryan

I think we should do this, or at least something similar. I'll keep this open for now as an alternative / complement to #7649

ReubenBond avatar Jan 12 '23 00:01 ReubenBond

Imo it'd be more flexible to specify timeout imperatively via code rather than attribute with a static timeout value

DunetsNM avatar Aug 30 '23 23:08 DunetsNM

It would be more flexible. How do you imagine specifying it? The two main options which I see are:

  1. Use a named policy, specified via an attribute
  2. Use a strongly typed policy, specified via a generic attribute

Specifying the type from the call site could be accomplished using RequestContext with either option.

ReubenBond avatar Aug 30 '23 23:08 ReubenBond

I'm not sure about the feasibility but I would imagine it like an extension method to either a task returned by a grain method (similar to .ConfigureAwait) or the IGrainFactory.GetGrain extension / overload

await grain.DoSomething().ConfigureGrainTimeout(TimeSpan);

or

var grain = grainFactory.GetGrain<...>(...).WithTimeout(TimeSpan);
await grain.DoSomething();

DunetsNM avatar Aug 31 '23 04:08 DunetsNM

I'm not sure about the feasibility but I would imagine it like an extension method to either a task returned by a grain method (similar to .ConfigureAwait) or the IGrainFactory.GetGrain extension / overload

await grain.DoSomething().ConfigureGrainTimeout(TimeSpan);

or

var grain = grainFactory.GetGrain<...>(...).WithTimeout(TimeSpan);
await grain.DoSomething();

That would even make the timeout different per call.

Another possibility could be to use Microsoft.Extensions.Options in some way, although I'm not sure how it would work to have options per grain type. Can you have IOptions<GrainOptions<TGrain>>, or perhaps GrainOptions could have an IDictionary<Type, SingleGrainTypeOptions>?

I'm presuming it would be beneficial to know before each invocation what the timeout will be, especially in the light of the [LongRunning] discussion. And if Orleans wants to support changing the timeout at runtime after startup, supporting IOptionsMonitor is always an option. (hah!)

P.S. I am also in favor of not being constrained by attributes that need constant values. I prefer things like this to be configurable somehow, but I would be okay with this configuration being "static".

ghost avatar Aug 31 '23 06:08 ghost

Having a .WithTimeout(x) extension on IGrain which returns a new grain reference could work. It's a little allocation-heavy for my liking, though, and the API feels a little awkward to me.

The await grain.DoSomething().ConfigureGrainTimeout(TimeSpan); option is not workable without some hackery or using a different return type for grain calls (eg, Rpc<T>/Request<T> instead of Task<T>/ValueTask<T>, etc). The core reason is that Task in .NET is hot: they start execution eagerly, before await. For contrast, IAsyncEnumerable<T> methods are cold: they do not begin execution until you begin enumerating the result.

As for attributes which reference a configurable policy, I think that is a viable approach. The difficulty will be in ensuring that the result is still very fast. Having a per-call dictionary lookup (eg, via DI) or additional allocations would be expensive. It can be done, but it will require a lot of work at the codegen level to generate efficient code.

ReubenBond avatar Aug 31 '23 16:08 ReubenBond