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

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

Closed
bill-poole opened this issue Jul 6, 2022 · 8 comments · Fixed by #8599
Closed
Labels
area-grains Category for all the Grain related issues help wanted Status: Fixed

Comments

@bill-poole
Copy link
Contributor

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.

@ghost ghost added the Needs: triage 🔍 label Jul 6, 2022
@rafikiassumani-msft rafikiassumani-msft added area-grains Category for all the Grain related issues help wanted and removed Needs: triage 🔍 labels Jul 7, 2022
@rafikiassumani-msft rafikiassumani-msft added this to the 4.0-Planning milestone Jul 7, 2022
@ghost
Copy link

ghost commented Jul 7, 2022

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.

@htxryan
Copy link

htxryan commented Dec 9, 2022

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.

@captainsafia captainsafia removed their assignment Dec 9, 2022
@ReubenBond
Copy link
Member

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

@DunetsNM
Copy link

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

@ReubenBond
Copy link
Member

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.

@DunetsNM
Copy link

DunetsNM commented Aug 31, 2023

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();

@ghost
Copy link

ghost commented Aug 31, 2023

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

@ReubenBond
Copy link
Member

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.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-grains Category for all the Grain related issues help wanted Status: Fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants