ZiggyCreatures / FusionCache

FusionCache is an easy to use, fast and robust hybrid cache with advanced resiliency features.
MIT License
1.65k stars 87 forks source link

[BUG] Mention correct way to call `GetValueAsync<T>(...)` in doc #191

Closed rmandvikar closed 6 months ago

rmandvikar commented 7 months ago

Describe the bug

The doc doesn't mention what is the correct way to call GetValueAsync<T>(...) method.

https://github.com/ZiggyCreatures/FusionCache/blob/main/docs/CoreMethods.md#getorsetasync

I'm not able to figure out what the difference in behaviors is for the two code snippets below.

To Reproduce

It looks like looking at the test cases code, below is the correct way to call.

// sample 1
public async Task<T> GetValueAsync<T>(object key, CancellationToken cancellationToken)
{
    var value = await fusionCache.GetOrSetAsync(
        (string)key,
        async ct =>
        {
            return await valueProvider.GetValueAsync<T>(key, ct);
        },
        token: cancellationToken);
    return value;
}

However, I've code as below, and I can't tell what happens in this case. Is this something that must be avoided? Or the upshot the same as sample 1 code?

// sample 2
public async Task<T> GetValueAsync<T>(object key, CancellationToken cancellationToken)
{
    var value = await fusionCache.GetOrSetAsync(
        (string)key,
        ct => // ct ignored
        {
            return valueProvider.GetValueAsync<T>(key, cancellationToken); // not awaited, but cancellationToken arg used
        });
    return value;
}

Note: I've ran this under different configuration for happy paths, unhappy paths (with different error %), and I'm seeing the same latency. However, I'd like to understand why I'm not seeing any difference.

Expected behavior

The doc should mention the correct way to call GetValueAsync<T>(...), and if the other variant has the same upshot or if a different upshot then describe that and why it should be avoided.

Versions I've encountered this issue on:

Screenshots n/a

Additional context n/a

jodydonetti commented 6 months ago

Hi @rmandvikar , and thanks for using FusionCache!

I'm not sure I'm following you here: the things you described are not specific for how to work with FusionCache, but how to call async methods in .net itself.

Normally the CancellationTokens are "chained", meaning that one present as a param in a method should be passed to a sub method call inside of it (if it does support it). For example in a controller action you can have a CancellationToken as a param, and use that when calling the database inside of the action.

Having said that, when you have a method that accept a lambda (like the factory in a GetOrSet call), the lambda should be designed such that it can accept a CancellationToken itself, for 2 main reasons:

  1. it is the signature of the lambda itself, because the signature is for a method that accepts a CancellationToken so it should be there as a param
  2. by being a param of the lambda you can avoid some capturing

In your 2 examples I can see 2 separate differences:

  1. in the first one the lambda is specified as async, and inside it does an await, while in the second one the lambda is not marked as async and it does not await, so in the second one the Task<T> returned by valueProvider.GetValueAsync<T>(...) will be returned directly. The difference in this case is mostly related to how exceptions that can be thrown will be handled by the runtime
  2. in the first one you are using the CancellationToken provided to the lambda, whereas in the second one you are directly using the outer one, therefore capturing it in a closure

But again, these are not related to FusionCache itself: the same things can be said about the controller/action example I've made earlier, so I don't think I should add them to the documentation.

Closing this for now, but let me know what you think and if I've missed something I'll gladly reopen it.