-
Notifications
You must be signed in to change notification settings - Fork 90
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
FusionCache.CompleteBackgroundFactory => Using "GetAwaiter().GetResult()" instead of ".Result" #258
Conversation
…etResult()" instead of ".Result" (it's better according to .NET Async guide: https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md)
@@ -400,7 +400,7 @@ private void CompleteBackgroundFactory<TValue>(string operationId, string key, F | |||
options.ReThrowBackplaneExceptions = false; | |||
|
|||
// ADAPTIVE CACHING UPDATE | |||
var lateEntry = FusionCacheMemoryEntry<TValue>.CreateFromOptions(antecedent.Result, options, false, ctx.LastModified, ctx.ETag, null); | |||
var lateEntry = FusionCacheMemoryEntry<TValue>.CreateFromOptions(antecedent.GetAwaiter().GetResult(), options, false, ctx.LastModified, ctx.ETag, null); |
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.
Whilst anything is better than .Result, GetAwaiter().GetResult() is still making things synchronous.
Could it not just be awaited?
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.
Good point, I haven't noticed that! Done.
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.
Whilst anything is better than .Result, GetAwaiter().GetResult() is still making things synchronous.
Could it not just be awaited?
Why? In this case we know for sure the task is already finished, why going through an extra async state machine & friends?
Hi all, I'll comment more later (sorry, super busy with my daily job) but this is a non-issue: some lines above I've already verified that the task is completed, so this will not block. If I haven't missed something it's like here (by Marc Gravell). |
Is it enough to not to migrate to async/await? I don't think so. According to .NET Async guide, a task which is usually already completed should be threated differently (using |
Hi @henriqueholtz , sorry for the delay.
The task in question is not usually already completed, it's the (user provided) factory to grab the fresh version of the data, and it can go from being instantaneous to taking seconds or minutes, and we don't have control over that. I'm checking if, at that moment, is already completed but that does not mean it usually is. Also, Having said that I see that the latest version of the PR is using async code directly: I'd like to check it better and do some tests, it may in fact work better. Let me check it and will come back with something soon. Thanks! |
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.
An additional await
means an additional async state machine and so on, which in this case (where we already know the task is completed) it's a waste.
Let's get back to GetAwaiter().GetResult()
and I think we can merge this.
This reverts commit 9418c07.
[Disagree and commit] I tend to disagree about using |
Why you disagree? I'd like to know! ps: remember that in this case we are talking about something that we know for sure is already completed, see here |
Btw, regarding
So yeah they are "completely equivalent". |
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.
LGTM
@jodydonetti I see that the task is already finished. But according to the references bellow, These advantages from
I can't understand why
Refs: |
Also, since what we are talking about is the user-provided factory, I can't just change the signature from
Nope: the task we are talking about is the one for the user-provided factory, and that can take anything from a couple of milliseconds up to minutes, we don't know and we can't possibly know. And we can't take a And we can't just change the signature to always be a To be clear: if I would create FusionCache from scratch today, I would have probably specified the signature of the factory to use
No, because:
look at the header text:
in particular the final part
As said, we don't know what the user-provided factory does: usually it is a call to a database, which is for sure non pre-computed, and 99.999% of the times non trivially compoted. Hope this helps, anyway I'm merging this. |
According to .NET Async guide, it's better to use
.GetAwaiter().GetResult()
than.Result
.