VincentVrijburg / moneybird-dotnet

A wrapper for the Moneybird API.
MIT License
6 stars 3 forks source link

Fix race condition for configurations #26

Closed GerardSmit closed 1 year ago

GerardSmit commented 1 year ago

This PR fixes two issues:

1. The configuration is being stored static

Currently all instances of MoneybirdClient share the same configuration. This can cause a race condition if there are multiple threads that creates their own MoneybirdClient with different configurations.

For example:

var firstTask = Task.Run(async () =>
{
    var firstConfig = new MoneybirdConfig { ClientId = "ClientA" };
    var firstMoneybirdClient = MoneybirdClient.GetInstance(firstConfig);
    await Task.Delay(1000);
    return await firstMoneybirdClient.Payment.GetPaymentByIdAsync("...", "...", "...");
});

var secondTask = Task.Run(() =>
{
    var secondConfig = new MoneybirdConfig { ClientId = "ClientB" };
    var secondMoneybirdClient = MoneybirdClient.GetInstance(secondConfig);
    return secondMoneybirdClient.Payment.GetPaymentByIdAsync("...", "...", "...");
});

var result = await Task.WhenAll(firstTask, secondTask);

var firstResult = result[0];
var secondResult = result[1];

In this case the firstMoneybirdClient will get the configuration of the secondMoneybirdClient and sends the request with the wrong credentials.

This has been fixed by giving each MoneybirdClient their own configuration instance, instead of being static.

2. It's not possible to pass the HttpClient

The MoneybirdClient always creates their own HttpClient. When you're using IHttpClientFactory, it's not possible to pass the HttpClient that the client should use. I've made the constructor public and added the HttpClient as argument.

I've also implemented IDisposable to dispose the HttpClient.

Example:

var factory = new HttpClientFactory();

var config = new MoneybirdConfig();
using var moneybirdClient = new MoneybirdClient(config, factory.CreateClient());

// Use moneybirdClient

Note: MoneybirdClient.GetInstance does not dispose the HttpClient since it's possible that a different thread is still using the HttpClient, causing all the requests to abort.

GerardSmit commented 1 year ago

Now that I think about it. The config is being stored in the end-points:

https://github.com/VincentVrijburg/moneybird-dotnet/blob/51913dbc4e20fd4215f40fcafd6f79f8143e3cba/src/Moneybird.Net/MoneybirdClient.cs#L53-L60

So changing the static configuration will not doing anything. It's not as critical than I thought but it can still happen when the MoneybirdClient instance is being constructed 🤔