CurseForgeCommunity / .NET-APIClient

A CurseForge API Client (For CurseForge Core)
MIT License
12 stars 3 forks source link

Problem with multithreading #1

Closed Cyl18 closed 2 years ago

Cyl18 commented 2 years ago

in readme.md

Please make note that the ApiClient inheirits of IDisposable, so that you dispose of it when you're done with it.

consider this code:

await Parallel.ForEachAsync(Enumerable.Range(1, 100), async (i, token) =>
{
    try
    {
        using var apiClient = new ApiClient("MY_API_KEY", "MY_EMAIL");
        var file = await apiClient.GetModAsync(238222);
    }
    catch (Exception e)
    {
        Console.WriteLine(e);
    }
});

it pops out some exceptions like this:

System.Threading.Tasks.TaskCanceledException: OperationCanceled
 ---> System.IO.IOException: net_io_read
 ---> System.ObjectDisposedException: ObjectDisposed_Generic
ObjectDisposed_ObjectName_Name, System.Net.Sockets.NetworkStream
   at System.Net.Sockets.NetworkStream.<ThrowIfDisposed>g__ThrowObjectDisposedException|63_0()
.......
   at CurseForge.APIClient.ApiClient.GET[T](String endpoint, ValueTuple`2[] queryParameters) in D:\Git\CurseForge.APIClient\ApiClient.cs:line 79
   at CurseForge.APIClient.ApiClient.GetModAsync(UInt32 modId) in D:\Git\CurseForge.APIClient\Mods.cs:line 22
   at Program.<>c.<<<Main>$>b__0_0>d.MoveNext() in C:\Users\cyl18\source\repos\TestDotNet6\TestDotNet6\Program.cs:line 15

in ApiClient.cs, the _httpClient is static, which means a single instance disposes would affect all existing instances https://github.com/CurseForgeCommunity/.NET-APIClient/blob/05caa75f202a3c483c8abbd65bb3be1dc97a911e/ApiClient.cs#L13 and a static HttpClient would cause extra problems like all ApiClient will use the same ApiKey

i have made a simple pull request to fix this problem :) (#2)

itssimple commented 2 years ago

Well, the point of the client, is to use a single API key. (Which could be handled with multiple instances as well, but that's beside the point)

The problem with creating a new per instance, could be port exhaustion.

But I could redo it to use IHttpClientFactory, which will handle pooling internally, while still allowing multithreading and avoid port exhaustion. Will work on it during lunch. (Edit: Forgot to turn on my server at home, so can't remote and fix it)

Thanks for notifying me of this though!