barnhill / barcodelib

C# Barcode Image Generation Library
Apache License 2.0
736 stars 239 forks source link

Dynamic Assembly memory leak #142

Closed udlose closed 2 years ago

udlose commented 2 years ago

There is a memory leak in the implementation of XmlSerializer usage. A new XmlSerializer(typeof(SaveData)) is created every time the following are called:

This usage creates a dynamic assembly every time the XmlSerailizer(typeof(T)) constructor is invoked. This results in a memory leak.

EDIT: This usage creates a dynamic assembly every time the XmlSerailizer(typeof(T)) constructor is invoked. This is not correct. The leak occurs in different constructors. I've been so used to having to correct these in other apps, I confused the dynamic assembly memory leak with a different constructor. My apologies.

barnhill commented 2 years ago

Destruction of the serializer still leaks?

udlose commented 2 years ago

I clarified the issue and corrected a statement I made. And in the case of the other (leaky) constructors (which you are not using here), the assembly that the XmlSerializer created still leaks - not the XmlSerializer instance itself. Dynamically created assemblies aren't cleaned up until the app domain is unloaded even if the object that created them has been GC'd - dynamically created assemblies are not considered a rooted reference.

Have a look here for more info: Dynamically Generated Assemblies

All this being said, creating an XmlSerializer instance is a little bit heavier than other framework types. And since you are always creating an instance that serializes the same type SaveData, it makes sense to create it once and hold onto it.

udlose commented 2 years ago

@barnhill I'm also not sure why you implemented IDisposable on SaveData as there are no managed or unmanaged resources in the class to dispose of. This pattern puts a little more pressure on the GC when you have unnecessary IDisposable implementations.