C#•14mo ago

❔ ✅ Can someone help me to change GetAwaiter().GetResult() to lazy?

I have this:

public class MessagingClientCache
{
    private readonly IMessagingClientFactory _messagingClientFactory;
    
    private readonly ConcurrentDictionary<string, IMessagingClient> _messagingClients = 
        new();

    public MessagingClientCache(IMessagingClientFactory messagingClientFactory)
    {
        _messagingClientFactory = messagingClientFactory;
    }
    
    public async Task<IMessagingClient> MessagingClientGet(string name) =>
        _messagingClients.GetOrAdd(
            key: name,
            valueFactory: _ => _messagingClientFactory.Create(name).GetAwaiter().GetResult());
}

public class MessagingClientCache
{
    private readonly IMessagingClientFactory _messagingClientFactory;
    
    private readonly ConcurrentDictionary<string, IMessagingClient> _messagingClients = 
        new();

    public MessagingClientCache(IMessagingClientFactory messagingClientFactory)
    {
        _messagingClientFactory = messagingClientFactory;
    }
    
    public async Task<IMessagingClient> MessagingClientGet(string name) =>
        _messagingClients.GetOrAdd(
            key: name,
            valueFactory: _ => _messagingClientFactory.Create(name).GetAwaiter().GetResult());
}

As I know GetAwaiter().GetResult() is not good and I should use lazy but I can't understand how it should look

29 Replies

SWEETPONYOP•14mo ago

public class MessagingClientCache                                                                      
{                                                                                                      
    private readonly IMessagingClientFactory _messagingClientFactory;                                  
    private readonly ConcurrentDictionary<string, Lazy<Task<IMessagingClient>>> _messagingClients =    
        new();                                                                                         
                                                                                                       
    public MessagingClientCache(IMessagingClientFactory messagingClientFactory)                        
    {                                                                                                  
        _messagingClientFactory = messagingClientFactory;                                              
    }                                                                                                  
                                                                                                       
    public async Task<IMessagingClient> MessagingClientGet(string name) =>                             
        await _messagingClients.GetOrAdd(                                                              
            name,                                                                                      
            _ => new Lazy<Task<IMessagingClient>>(() => _messagingClientFactory.Create(name))).Value;  
}

public class MessagingClientCache                                                                      
{                                                                                                      
    private readonly IMessagingClientFactory _messagingClientFactory;                                  
    private readonly ConcurrentDictionary<string, Lazy<Task<IMessagingClient>>> _messagingClients =    
        new();                                                                                         
                                                                                                       
    public MessagingClientCache(IMessagingClientFactory messagingClientFactory)                        
    {                                                                                                  
        _messagingClientFactory = messagingClientFactory;                                              
    }                                                                                                  
                                                                                                       
    public async Task<IMessagingClient> MessagingClientGet(string name) =>                             
        await _messagingClients.GetOrAdd(                                                              
            name,                                                                                      
            _ => new Lazy<Task<IMessagingClient>>(() => _messagingClientFactory.Create(name))).Value;  
}

is it correct? maybe even this:

public Task<IMessagingClient> MessagingClientGet(string name) =>                                 
    _messagingClients.GetOrAdd(                                                                  
        name,                                                                                    
        _ => new Lazy<Task<IMessagingClient>>(() => _messagingClientFactory.Create(name))).Value;

public Task<IMessagingClient> MessagingClientGet(string name) =>                                 
    _messagingClients.GetOrAdd(                                                                  
        name,                                                                                    
        _ => new Lazy<Task<IMessagingClient>>(() => _messagingClientFactory.Create(name))).Value;

mtreit•14mo ago

Creating a Lazy<T> and immediately de-referencing the .Value property makes the use of Lazy<T> meaningless. Do you understand what Lazy<T> is intended for?

SWEETPONYOP•14mo ago

hm yes.. I should remove .Value

mtreit•14mo ago

Probably you want something like this:

public Task<IMessagingClient> MessagingClientGet(string name)
{
    var lazy = _messagingClients.GetOrAdd(
        name,
        _ => new Lazy<Task<IMessagingClient>>(() => _messagingClientFactory.Create(name)));

    return lazy.Value;
}

public Task<IMessagingClient> MessagingClientGet(string name)
{
    var lazy = _messagingClients.GetOrAdd(
        name,
        _ => new Lazy<Task<IMessagingClient>>(() => _messagingClientFactory.Create(name)));

    return lazy.Value;
}

SWEETPONYOP•14mo ago

hm but I can do this:

public Lazy<Task<string>> MessagingClientGet(string name) =>    
    _messagingClients.GetOrAdd(                                 
        name,                                                   
        _ => new Lazy<Task<string>>(() => ReturnString(name)));

public Lazy<Task<string>> MessagingClientGet(string name) =>    
    _messagingClients.GetOrAdd(                                 
        name,                                                   
        _ => new Lazy<Task<string>>(() => ReturnString(name)));

and then just:

var a = await cache.MessagingClientGet( "sou" ).Value; 
var b =  await cache.MessagingClientGet( "sou" ).Value;

var a = await cache.MessagingClientGet( "sou" ).Value; 
var b =  await cache.MessagingClientGet( "sou" ).Value;

or it is bad idk @mtreit can we say that Lazy is better than GetAwaiter().GetResult()?

mtreit•14mo ago

Anything is better than GetAwaiter / GetResult

SWEETPONYOP•14mo ago

thanks for helping!

cap5lut•14mo ago

btw, why not just a ConcurrentDictionary<string, Task<IMessagingClient>>?

SWEETPONYOP•14mo ago

we cant await in value factory

cap5lut•14mo ago

as in

public class MessagingClientCache
{
    private readonly IMessagingClientFactory _messagingClientFactory;
    
    private readonly ConcurrentDictionary<string, Task<IMessagingClient>> _messagingClients = 
        new();

    public MessagingClientCache(IMessagingClientFactory messagingClientFactory)
    {
        _messagingClientFactory = messagingClientFactory;
    }
    
    public Task<IMessagingClient> MessagingClientGet(string name) =>
        _messagingClients.GetOrAdd(
            key: name,
            valueFactory: _ => _messagingClientFactory.Create(name));
}

public class MessagingClientCache
{
    private readonly IMessagingClientFactory _messagingClientFactory;
    
    private readonly ConcurrentDictionary<string, Task<IMessagingClient>> _messagingClients = 
        new();

    public MessagingClientCache(IMessagingClientFactory messagingClientFactory)
    {
        _messagingClientFactory = messagingClientFactory;
    }
    
    public Task<IMessagingClient> MessagingClientGet(string name) =>
        _messagingClients.GetOrAdd(
            key: name,
            valueFactory: _ => _messagingClientFactory.Create(name));
}

and then var client = await cache.MessagingClientGet("blah"); u dont have to

SWEETPONYOP•14mo ago

hmmm and can we say that this is atomic operation?

cap5lut•14mo ago

u can also can use just valueFactory: _messagingClientFactory.Create, no need to create a closure on name, because the parameter u omit in the lambda is already the key this could be indeed a problem, didnt think about that

SWEETPONYOP•14mo ago

Create method actually creates the object. Is there any chance to create this object twice? We don’t await in factory so… thats why i thinked about lazy

cap5lut•14mo ago

yeah, i guess lazy has its usage here, tho i wouldnt expose it on the api surface, i would still just return a Task<IMessagingClient>

SWEETPONYOP•14mo ago

hm and what if the method is called in multiple threads?

cap5lut•14mo ago

i was thinking about if a local method like async Task<IMessagingClient> CreateMessagingClient(string name) => await _messagingClientFactory.Create(name); would be enough to not run any potential synchronous code in _messagingClientFactory.Create() immediately doesnt really matter as long as the value factory method doesnt run to long hmmm, maybe it doesnt matter at all, lemme start vs really quick

SWEETPONYOP•14mo ago

imagine you call this method in 2 threads first thread go to value factory second thread go to value factory method will called twice i mean method in value factory also _messagingClientFactory.Create is used to create endpoint so endpoint can be created twice

cap5lut•14mo ago

yeah, was just checking if the factory method would be called twice or not and it will be indeed, so the Lazy approach is indeed better/without race condition

SWEETPONYOP•14mo ago

hm and what about GetAwaiter.. is it better than lazy? both block the thread

cap5lut•14mo ago

so this one would be the best way GetAwaiter blocks for the whole time the task runs, Lazy will only block for the first synchronous part of the task execution and the rest will be ur normal async code the main point is that the start of the task isnt inside the value factory, so even if the factory method runs twice (this creating 2 Lazys), only one instance makes it into the dictionary and the second instance will be omitted. thus the call to lazy.Value which will trigger the actual task creation, etc, will be only once and everything thats blocked on there will get the Task instance as soon as its execution hits the first await internally

SWEETPONYOP•14mo ago

imagine service will be running some month using GetAwaiter we block only one time and Lazy blocks everytime

cap5lut•14mo ago

im pretty sure Lazy uses some non blocking method once the initialization has been completed, so its only for that small amount of time that its blocking and then never again with GetAwaiter() u could end up with all threads being blocked and waiting for a task to be scheduled to be completed, which cant happen because all threads are busy. with Lazys blocking method here, it isnt a task that will be scheduled to finish completion but its blocking the thread to complete it itself and thus unblocking other waiting threads as well

SWEETPONYOP•14mo ago

do you think that losing resources on each of the tens of thousands of calls during the months of the process life is better than blocking the thread once for a couple of nanoseconds? Or I didn't understand something

cap5lut•14mo ago

the difference is the risk of a dead lock being there vs the risk being not there and with the GetAwaiter() in the value factory u would end up with the same problem as just using ConcurrentDictionary<string, Task<..>>, the actual _messagingClientFactory.Create(name) method could be invoked several times

SWEETPONYOP•14mo ago

hm really? even with GetAwaiter().GetResult() we can create two or more endpoints with the same name?

cap5lut•14mo ago

yes, because the valueFactory could be invoked multiple times, just ONE of its results will make it into the dictionary and will returned as result. but because _messagingClientFactory.Create(name) would be called from within the valueFactory, u just create more and omit them again new Lazy(....) just creates an instance, it doesnt call _messagingClientFactory.Create(name) yet, so even if multiple are created, nothing happens yet. only one instance makes it into the dictionary and that instance will be returned for all threads. the lazy.Value after getting it back from the dictionary is then the same instance for all threads. all these threads will be then blocking and one of these will execute the Lazys factory method, store its result and then all threads become unblocked and have the result note that the result here is not the finished task, but the created and scheduled task. if then later one or more threads get the Lazy instance, its already initialized, so its non-blocking and they get the task instantly, which they then can await here is a small example, showing whats going on: https://sharplab.io/#v2:C4LgTgrgdgNAJiA1AHwAICYCMBYAUKgBgAJVMA6AYQHsAbGgUwGNgBLKqAZ0vcYjDHpRgAbjyES5ACoALAQEM4LKAHNRuPADc5YIouZEAvESj0A7kWpRe/QcAAiLZmyjaAngB5SBGEQAycgC8PKgAjACsmYAA+KIAKAEo1fyD3UIjmKKIANTkaCHoAMTlmKjBXWK9jOQBbeni8AG88IhaJAE5YgBIAIkZ5VhUiGkDXIgAzUqIGl1qAX27E5taZeTgyAGUGegAHWMwCAkXcVpIAdmMzPxHU8Mi4hMMopZam45PW0g6evvo5AeUpjN6PMju9lrJfmtNvQdnsDqCwe0ur1+vQ4ICasCFmpEWcLuY0pEEjjWrMjrM8JptEQOPRqoZ8UR1nS5NtpKV6LFvER0EctDpgBCFJgGSZzCtIbEHgYnrhXidadUyAB1OQsYAAeRMxOeRH5QxGDL0wDIAHF6JqwABBOBwWLdJQcYByKz0bo+HJ5QrFYClVwIloAfQZwyCZE9+TUZLU+sFq3QosuEoUUvij0ausVKrVmu1Ab11NDoyMxrNFo11tt9sdztd7uyuXyRRKZXzwaMRfDjfoUaOeAA9P2ac6wMAiHHIRwiC70YIOHx6OPpPRRtpF6tRr6iJAoNOx4LFxxMeOWLU8BPhRsR8AdbgL3B0FftDejsmoVtdvtDmoswAlegMHItKxLyagDkOpg5uMkwHkuqxTluYxKCwHDSOeQpwOQABSVBKLe96PjheGJEAA== the Thread.Sleep() calls and the semaphore are in there to get the timings right, so that the 2 threads would run "at the same time" (maybe not exactly at the same time, but their executions overlap enough)

SWEETPONYOP•14mo ago

ah I understand now, thanks for helping me

cap5lut•14mo ago

glad i could help o7

Accord•14mo ago

Was this issue resolved? If so, run /close - otherwise I will mark this as stale and this post will be archived until there is new activity.

Gaming

Programming

❔ ✅ Can someone help me to change GetAwaiter().GetResult() to lazy?