C# : Whether this approach is thread-safe

Question

I have a service class, which does request to some external service. That external service requires sessionId, time of live of this sessionId is unknown (can be some seconds, can be minutes, can be hours, can be days. Can be 1 request, can be 1000 requests. Nobody knows it. We can't link to it at all).

I created a method, which get sessionId:

    private async Task<string> LoginAsync()
    {
        using (HttpClient clientLogin = new HttpClient())
        {
            //....
        }
        return sessionId;
    }

I declared the following variable inside my class

public class BillComService : IBillComService
{
    private static Lazy<string> SessionId;

and init it in constructor:

    public BillComService()
    {
        if (SessionId == null)
            SessionId = new Lazy<string>(() => LoginAsync().Result);
        //....
    }

then I use it inside my methods:

    private async Task<T> Read<T>(string id, string endpoint)
    {
        var nvc = new List<KeyValuePair<string, string>>
            {
                new KeyValuePair<string, string>("devKey", devKey),
                new KeyValuePair<string, string>("sessionId", SessionId.Value)
            };

ok, it's fine. Think about sessionId is expired. Method with throw BillComInvalidSessionException

Right now I wrote the following method to repeat my request after calling LoginAsync method if sessionId is expired. Common method:

    private async Task<T> Retry<T>(Func<Task<T>> func)
    {
        try
        {
            return await func();
        }
        catch (BillComInvalidSessionException)
        {
            SessionId = new Lazy<string>(() => LoginAsync().Result);
            return await Retry(func);
        }
    }

and for Read:

    private async Task<T> ReadWithRetry<T>(string id, string endpoint)
    {
        return await Retry(async () => await Read<T>(id, endpoint));
    }

so, my public method is:

    public async Task<Customer> GetCustomerAsync(string id)
    {
        return await ReadWithRetry<Customer>(id, "Crud/Read/Customer.json");
    }

it works and work fine. But I'm not sure, whether it's thread-safe :)

Answer 1

Your code doesn't perform any kind of synchronization and allows for one or more logins issued in parallel. Whether this is a problem or not is entirely dependent on the remote implementation issuing the session IDs.

Here's the 3 main scenarios I can think of:

1. The remote login process synchronizes the session ID generation per developer's key and is guaranteed to return the same session ID for concurrent logins.

2. The remote login process will issue a new session ID per login, even in contention scenarios, but all session ID will remain valid.

3. The remote login process will issue a new session ID per login, but each subsequent logins are invalidating previously issued session IDs for a given developer's key.

With scenarios 1 & 2, the worst that can happen are inefficiencies (e.g. unnecessary network IO) due to concurrent login processes.

However, with scenario 3 then things could get ugly as you may end with many retry loops since session IDs could get invalidated right after being issued. The more concurrent requests there are with an invalid session and the worst it would get.

If you want to make sure that your code is safe independently of the remote implementation then you would have to synchronize the SessionId read & writes, which involves synchronizing the login process as well. Still, the remote implementation behavior is part of their service contract so it can also be reasonable for your implementation to take that behavior into consideration.

Answer 2

No, it's not thread safe.

You have a race when multiple threads set the shared SessionId field in parallel and another one when one thread is setting SessionId while other threads are using it.

BTW: it seems you are missing an wait when calling LoginAsync().