-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Support cancellation tokens in grain method signatures #1599
Conversation
So the 2 questions (at least) that we need to decide upon are:
|
var cancelled = ctw.CancellationToken.IsCancellationRequested; | ||
if (!cancelled) | ||
{ | ||
CancellationTokenManager.RegisterTokenCallbacks(ctw); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets try to maintain layering. Serialization should only serialize, not call upwards into silo logic (we are trying, as best as we can, to maintain acyclic graph of logical dependencies).
You can do CancellationTokenManager.RegisterTokenCallbacks when the request is sent, from Outside/Inside grain client SendRequest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially this was made with Action
callback, which maintained the layering, to avoid unnecessary computations in case of local grain call, and then replaced due to #1569 (comment). But if you think that now the cost of grain extension call and CancellationTokenSource
allocation is neglectible compared to the code readability & supportability I will move it to SendRequest
methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just don't want serializer to call back into the runtime logic. Serializer may change, be replaced by another one, ... It should only serialize and have zero side effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Would it be fine to move this methods into CancellationTokenWrapper
and mark them with [SerializerMethod]
, [DeserializerMethod]
.. attributes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes.
|
Regarding reentrancy - I am not sure what you are saying is correct. The OnCancelled is a callback, you don't know on what thread it is called. If you do know, please provide a reference and test. |
Can you please explain what is the whole story about disposing. I don't understand what we are disposing and what is |
About disposing: the token cancellation callback accepts state object and |
Who initiates the disposal? The source at the sender? So we will be doing an extra grain call to dispose each token? Unacceptable of course. |
No, we're doing extra grain call for bunch of tokens (25 currently). |
Regarding reentrancy - can you please descibe about which `OnCancelled' callback do you mean? |
Regarding reentrancy - I mean user defined callback on remote token. That is part of what I called "programming model". I still don't get it about remote disposing - why do we need it? Why doing any at all? |
It's not really about disposing, it's rather about freeing references to |
Of course we need to clean up resource (unused cancellation tokens in the target grain extension). But we don't need to send remote msgs for that. We can cleanup locally. We use those "tricks" all the time in the runtime - remote calls are expensive. Need to do them only when absolutely necessary. |
That would be an easy solution - just to store WeakReference and wait for token to go out of skope, but token can go into long runnig task, or db query, and the guarantees that it will be cancelled in any case are needed. |
what do you mean? Why would you want to cancel it as long as application holds a reference to it? Plus, most importantly, there are no resources associated with the token, except the token itself. As long as app holds it, you can't get rid of the token. The only other "resource" is the entry in the extension dictionary, which is minor/negligible. Unless, there is something else that you mean - the source (whom ever created the cancellation source) can dispose the source/token and that operation has a semantic meaning in the application (like it is important to the app that if the source was disposed, any check on the token will throw DisposedException). I hope you don't mean that. |
I was talking about the entry in the extension dictionary. 1 million entries is taking 500 mb of memory, so I thought this could potentially be an issue, but, as it seems, my assumptions were wrong. Will remove the remote disposing part. |
But as long as the token itself is alive, we have to keep something for its management and we can't get rid of it as long as app can use it to cancel via it. Correct? So if app sends million tokens and wants to keep using them all, I am not sure what we can do better. One other option is to say that token is bound to the request which it came with - when this request is replied/times out, we will also dispose the token. But that means the token cannot be used for some longer background operations, that stay around also after the request timed out. Regardless, I think we should design the cancellation orthogonality to this. |
About callback reentrancy and "programming model": lots of libraries and .Net framework methods accepts |
Can you guarantee single threaded execution? If not, I would vote against the whole feature, so just not to sacrifice, by any means, the single threaded execution. |
Is the callback called synchronously with the cancell call? |
Just to chime in with the observation that this has been implemented in a few places already. Would it be worth taking a look to widen perspective, so to speak?
Maybe the way to go is to stash it in the context as wrapped one, but if there is a These handlers wouldn't respect the Orleans turn-based guarantees, but it might be OK, because cancellation might have been requested because the grain has been perceived to be stuck. One thing to think here is that how are linked tokens handled, I would imagine they'd need to go via a wrapper and so would the wrapper register itself as one of the cancellation handlers? I'm not sure how this works in this current implementation. Here are links to docs by the original implementators (including a pdf one, at the end it describes how to add Sorry if I pollute discussion now, this is somewhat a half-baked writing. |
Callback is executed as part of the cancel method in the grain extension by runtime scheduler worker pool thread, it can interleave with original grain methods but execution remains single threaded. |
I was not asking about how it executes in Orleans. I was asking about the Cancellation token itself. |
Im talking about Cancellation token itself - registered callbacks are executed either synchronously with the |
How did you come to that conclusion? Do you have documentation, evidence? Where did you see that the callback runs on the current Task Scheduler? |
Added test for cancellation callbacks execution context. |
Looks like it is called synchronously: |
|
||
internal void AddGrainReference(GrainReference grainReference) | ||
{ | ||
_targetGrainReferences.AddOrUpdate(grainReference.GrainId, id => grainReference, (id, ignore) => grainReference); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use TryAdd
instead of AddOrUpdate
? it shouldn't matter which instance of GrainReference you use as long as they both reference the same grain, correct?
@dVakulen this is great! as you saw, I only had some minor comments for you to address. As we expected, load test results show no impact with your change in it (again, we did not add a scenario that uses cancellation tokens, I just verified that code without them is unaffected). I'll merge once you address those minor things. |
Julian, please leave me the honor to merge this pr (after all your comments are addressed of course). 233 comments and 3 months of work, including multiple rounds of redesign, I think I deserve it. 😀 |
Haha, of course, thanks a lot @gabikliot! |
@dVakulen This is a great one. 👍 A note for the the future, as per the original scenario, a great test and an example use case would to add an integration test scenario with Microsoft.Owin.Testing that does |
@dVakulen @gabikliot This is an epic PR that adds a non-trivial feature that went through a number of iterations. Your effort is very much appreciated! @veikkoeeva We don't need http in the test scenario, do we? Just making a call from an Orleans client and cancelling it should be enough I think. |
ef140e5
to
b2130fd
Compare
Comments addressed, rebased and squashed. |
Thanks, @dVakulen . There was just one unaddressed comment. But considering it is minor, I'm OK if @gabikliot merges this, and I can address it in a separate PR |
@gabikliot please do the honors! :) |
@dotnet-bot test this please |
Thank you @dVakulen ! |
Thank you all who had participated in this work, especially @gabikliot and @ReubenBond for thorough reviews and advises; lots of lessons learned, will try to make my next PR's more refined. |
@dVakulen Nah, don't worry, it was refined enough. It is very typical to go through a number of iterations for a significant change like this. Thanks again for slogging through the long process! |
Implemented using grain extensions accordingly to the #1569 (comment)