Use HttpClientFactory

[KeithHenry]

Currently you create an HttpClient whenever you need it without a PooledConnectionLifetime:

OpenAI-API-dotnet/OpenAI_API/EndpointBase.cs

Line 63 in dd49841

HttpClient client = new HttpClient();

This means a client per request that opens an HTTP connection but doesn't close it until the GC finalises it.

As per the guidelines either:

• Use a static or singleton HttpClient instance with PooledConnectionLifetime set to the desired interval, such as two minutes, depending on expected DNS changes. This solves both the port exhaustion and DNS changes problems without adding the overhead of IHttpClientFactory. If you need to be able to mock your handler, you can register it separately.

• Using IHttpClientFactory, you can have multiple, differently configured clients for different use cases. However, be aware that the factory-created clients are intended to be short-lived, and once the client is created, the factory no longer has control over it.
  The factory pools HttpMessageHandler instances, and, if its lifetime hasn't expired, a handler can be reused from the pool when the factory creates a new HttpClient instance. This reuse avoids any socket exhaustion issues.
  If you desire the configurability that IHttpClientFactory provides, we recommend using the typed-client approach.

[OkGoDoIt]

Hmm, good idea. I haven't seen this guidance before, but it makes sense. Things are never as simple as they seem like they should be 😅

I'll look into improving this along with the next set of updates. Currently I am swamped with other work. If you want to submit a PR, that could help get this fixed faster.

[gotmike]

does this require the project support dependency injection throughout?

[KeithHenry]

@gotmike you could call the services directly to access the HttpClientFactory or use your own singleton with PooledConnectionLifetime, both would be better, but the former is messy and the latter will basically mean you having your own connection pool for requests from a fairly slow API.

The fact you don't support DI rules out using this package in my organisation (as it currently is) - we have lots of dependencies, they're only manageable because they all use .NET's DI model.

The problem is that it's a breaking change.

[gotmike]

I think we should support DI but this is a big change. Is there a way to implement DI in a new method and also provide the non-DI method alongside to be later deprecated?

[KeithHenry]

Yes, but not in a way that doesn't have this bug.

You could just add a DI extension method around new OpenAI_API.OpenAIAPI("...");, that would give you something that looks like DI.

However, without the HttpClient coming in from DI (from the factory) you're still going to exhaust the connection pool on servers. Desktop/console are probably fine as a few open unused HTTP over TCP connections can be handled, but on a server with ~100 users making requests within a few seconds of each other it's just going to hang.

DI would also make it easier to fix issues like #48, as you could use the tools DI gives you.

I've summitted a PR #44, it would have to be a v2 with breaking changes, possibly leaving you supporting v1 and v2 (although the endpoint implementations don't change).

You could fix this without DI, but you're probably looking at a singleton HttpClient.

[OkGoDoIt]

I started implementing PooledConnectionLifetime but it turns out it is not available in .NET Standard and .NET Framework, unfortunately. I can make a static HttpClient, I just can't set the PooledConnectionLifetime. My understanding is that won't be enough since the default PooledConnectionLifetime is infinite.

And DI is a whole can of worms that I'd rather avoid if possible. I get the engineering benefits but it just makes programming so messy and painful. If there's a way to add DI without changing the end-user SDK surface area then sure, but I'd hate to have to foist DI on all users of this SDK.

I don't see a way to implement this cleanly without sacrificing compatibility or ease of use

[OkGoDoIt]

Does disposing the HttpClient after each use solve the issue? @KeithHenry

[KeithHenry]

@OkGoDoIt that's even worse, then it doesn't pool connections at all, it just spins one up (with overhead) and then shuts it down (also overhead). One user is fine, 100 is a blocked up.

HttpClient is not well designed.

DI making programming messy and painful is wrong though. DI is much easier. Especially if you have lots of micro services.

The app I'm looking using OpenAI in already uses around 20 injected services. I set their config up when the program starts and then call them when I need them.

The instantiate with a key every time pattern means loading up the config every time, or keeping the config and passing it around (with all its secret keys and service endpoints). DI is just much much easier, to the point that we wouldn't add a dependency that didn't support it - 20 DI services and 1 indiosyncratic one is twice the code and maintenance of 21 DI services.

Also, anyone on .NET Core, or any mainstream .NET since 5 is using DI by default. It's been the out-the-box default for about 3 years now. By not supporting it you're stuck only really being an option for the legacy .NET 4.8 apps that don't want to upgrade to 5.

[gotmike]

this is how i understand it as well. DI is the new standard.

there is something to be said for providing a bridge between "old tech" (meaning standard/framework) and "new tech" (meaning the latest release of an AI engine) but it does seem like that is prob the exception rather than the rule. but i feel like we should be able to provide DI functionality and also support non-DI as well, with different endpoints. no?

[KeithHenry]

@gotmike I had an idea for that, as long as you're happy leaving this issue as a potential footgun for those users: dafb02e

[jodendaal]

@gotmike you could call the services directly to access the HttpClientFactory or use your own singleton with PooledConnectionLifetime, both would be better, but the former is messy and the latter will basically mean you having your own connection pool for requests from a fairly slow API.

The fact you don't support DI rules out using this package in my organisation (as it currently is) - we have lots of dependencies, they're only manageable because they all use .NET's DI model.

The problem is that it's a breaking change.

If your using .net core 6 or above you could try my package. It was the main reason I created it. Every OpenAI.net client I have seen has this same issue. Whenever I see new HttpClient it a 🚨. I have had to fix this issue in many production systems. It's not obvious I til one day your API /App stops working due to socket exhaustion or sometimes when DNS changes if using static. @KeithHenry

[alexandrereyes]

Here is an inspiration:
https://github.com/eealeivan/mixpanel-csharp/wiki/Configuration

you can expose a delegate so the developer can control how HttpClient behaves

MixpanelConfig.Global.AsyncHttpPostFn = async (url, stringContent) =>
{
    // TODO: Your async HTTP POST method
    return true;
};

HttpClientFactory is a must-have if you're using ASP.NET Core, otherwise, you'll face socket exhaustion