FusionCache.CompleteBackgroundFactory => Using "GetAwaiter().GetResult()" instead of ".Result"

[henriqueholtz]

According to .NET Async guide, it's better to use .GetAwaiter().GetResult() than .Result.

[jodydonetti]

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

[henriqueholtz]

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 ValueTask for example).

[jodydonetti]

Hi @henriqueholtz , sorry for the delay.

I don't think so. According to .NET Async guide, a task which is usually already completed should be threated differently (using ValueTask for example).

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, ValueTask is preferable over Task to avoid allocations, but it does not have anything to do with it being usually completed, and it's a task because that is the signature for the factory which cannot be changed now (it's a breaking change).

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!

[henriqueholtz]

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.

[Disagree and commit] I tend to disagree about using GetAwaiter().GetResult() instead of async/await + ValueTask (maybe it represent a gap in my knowledge). Even though it's done!

[jodydonetti]

[Disagree and commit] I tend to disagree about using GetAwaiter().GetResult() instead of async/await + ValueTask (maybe it represent a gap in my knowledge). Even though it's done!

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

[jodydonetti]

Btw, regarding .Result vs .GetAwaiter().GetResult() see here:

Is there a functional difference between “task.Result” and “task.GetAwaiter().GetResult()”?

... If the task ends in the RanToCompletion state, these are completely equivalent statements ...

So yeah they are "completely equivalent".

[henriqueholtz]

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

@jodydonetti I see that the task is already finished. But according to the references bellow, ValueTask is useful for async operations which the result is already completed/computed (exactly the scenario right here, isn't?).

These advantages from ValueTask compared to a simple Task isn't enough to avoid your concern (repeated below)?

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. [...]

I can't understand why GetAwaiter().GetResult() is better than ValueTask+async/await in this scenario. Additional phrase which comes from references:

ValueTask: It does not use any extra threads as a result. It also does not allocate an object on the managed heap.

Refs:

• .NET Async Guide
• Blog post "Understanding the Whys, Whats, and Whens of ValueTask"
• etc

[jodydonetti]

@jodydonetti I see that the task is already finished. But according to the references bellow, ValueTask is useful for async operations which the result is already completed/computed

As already mentioned:

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, since what we are talking about is the user-provided factory, I can't just change the signature from Task<T> to ValueTask<T> without breaking all existing code out there, it's not doable.

(exactly the scenario right here, isn't?)

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 Task, which again can take anything from milliseconds to minutes, and if in one case it finishes quickly turn that into a ValueTask because it's better when "the operation already completed/computed", which again is not the case.

And we can't just change the signature to always be a ValueTask because changing the signature of the factory would be a breaking change.

To be clear: if I would create FusionCache from scratch today, I would have probably specified the signature of the factory to use ValueTask<TResult> instead of Task<TResult>.

These advantages from ValueTask compared to a simple Task isn't enough to avoid your concern (repeated below)?

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. [...]

No, because: 1) as said we can't change the signature of the factory from Task to ValueTask 2) here we can even skip the await completely, Task or ValueTask that is, since we already know the task is completed (but again, not "in general" and only after having checked that and on a case-by-case basis)

Refs:

• .NET Async Guide

look at the header text:

Prefer Task.FromResult over Task.Run for pre-computed or trivially computed data

in particular the final part

for pre-computed or trivially computed data

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.