Potential memory leak and deferred garbage collection cycle identified

[thesobercoder]

There are a few potential memory leaks and deferred garbage collection cycle that I identified. I thought of bringing this to community's notice. Also, I will take this opportunity to suggest some better coding practices.

1. The following classes implement the IDisposable interface. If we don't call the Dispose method on them ourselves, then we potentially leave it to the garbage collector to dispose of and then cleanup the memory. This typically happens in 2 cycles of garbage collection(The garbage collector has 3 cycles). However, if we call the dispose method ourselves before exiting a block scope, the garbage collector will then take just one cycle to clean up the memory and end up doing less work. We have to remind ourselves that garbage collection is a very expensive process).

HttpResponseMessage - Docs
HttpRequestMessage - Docs
HttpClient - Docs

2. Newing up the HttpClient in the construction is typically a bad practice. It is always better to rely on Dependency Injection. The typical way to implement it is using this would be to accept IHttpClientFactory in the constructor.

IHttpClientFactory - Docs

then add dependency injection to inject a version of the IHttpClientFactory.

Microsoft.Extensions.DependencyInjection - Nuget

then create a ServiceCollectionExtensions static class and create a method like this -

// Clients of this Nuget package need to utilize this before using the classes.
public static IServiceCollection AddAppWrite(this IServiceCollection serviceCollection, IConfiguration configuration)
{
    // This should add the IHttpClientFactory that you want to register with proper retry policies using Poly.NET
   // This method should also house any other interface and class registrations with the DI system that are needed.
  // If it were me, I would also implement an IAppWriteUriProvider which has methods that returns the URI of all the areas that are needed and inject an implementation here.
}

and then use the IHttpClientFactory.CreateClient method to create the actual HttpClient. This typically is also an extension method. Not the HttpClient is a class that needs eager disposing.

IHttpClientFactory.CreateClient - Docs

Also, I would never expose the underlying HttpResponseMessage directly to the consumer, but instead return the string and let abstract away any other Http only resemblance. The reason I say this is because tomorrow you guys might implement the APIs using gRPC but to the consumer, it should not feel like anything has changed.

I apologize for all the ranting. My only intention is to really help the community with whatever knowledge I have.

[TorstenDittmann]

Hey 👋🏻

No need for the apologies, this is no rant and more like constructive feedback 🙂

Keep in mind that this SDK hasn't been officially announced yet is remains here for public testing 👍🏻

You are right with all the points, especially with the HttpResponseMessage. We are going to add "Response Models" to all the SDK's with strongly typed languages soon. For this we will introduce Classes for every response models from the server - would you still prefer having the string returned and handle everything on your own or Objects/Classes provided by the SDK?

All the other points are added to my ToDo list and will be implemented before the official release 🙏🏻

Thanks a lot for pointing these problems out, my C# knowledge comes from my first apprenticeship many years ago and is very dusty - so every feedback is very welcomed!

[thesobercoder]

I always prefer strongly typed classes. Like I said before if you guys ever shift to gRPC, the client wouldn't notice a difference.

[xavidram]

@thesobercoder Taking into account all the great points you mentioned in your initial post. Will creating Typed HttpClient services cause memory leaks as well?

I started working on a .NET implementation for appwrite for my own personal project and set up the typed client like this:
https://github.com/aplos-creative/Appwrite.NET/blob/master/Appwrite.NET/AppwriteClient.cs

Although I could have configured the IHttpClientFactory here:
https://github.com/aplos-creative/Appwrite.NET/blob/master/Appwrite.NET/ServicesExtensions.cs

And then referenced it in each service by calling the _httpFactory.CreateClient("clientname") in order keep it named. I wanted to use a Typed Client to only expose methods that will handle exceptions properly and have to keep re-using snipets of code in each endpoint call to handle '400's exceptions.

[thesobercoder]

@xavidram It will not cause a memory leak as long as we remember to call dispose method as early as possible. The idea is to make these HttpClient objects method scoped instead of class scoped. This is why IHttpClientFactory should be used to create a client inside each method as opposed to having one for the entire class. Also, remember the HttpClient and other disposable types must inside an using statement.

[xavidram]

Biggest things I want to prevent is socket exhaustion using the HttpClient factory. I'm going to look into swapping this implementation over to use the IHttpClientFactory and see how that works out.

Thanks.