Skip to content

DotNetXmlSerializer leaks memory #1988

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Timo-Weike opened this issue Dec 28, 2022 · 3 comments
Closed

DotNetXmlSerializer leaks memory #1988

Timo-Weike opened this issue Dec 28, 2022 · 3 comments
Labels

Comments

@Timo-Weike
Copy link

Timo-Weike commented Dec 28, 2022

DO NOT USE ISSUES FOR QUESTIONS

Describe the bug
The default xml-serializer DotNetXmlSerializer leaks memory in the form of dynamic assemblies.
As stated in the msdn for XmlSerializer, each instance of XmlSerializer should be cached for later use if it was not created by one of the mentioned self-caching constructors.

var serializer = new XmlSerializer(obj.GetType(), root);

To Reproduce
Run

var serializer = new DotNetXmlSerializer();

for (var i = 0; i < 1_000; i++)
{
    var res = serializer.Serialize(someObj);

    Console.WriteLine(AppDomain.CurrentDomain.GetAssemblies().Count());

    Thread.Sleep(100);
}

and start a memory profiler or take a memory dump after sometime.

Expected behavior
Reuse serializers.

Stack trace
Copy the full stack trace here if you get an exception.

Desktop (please complete the following information):

  • OS: Linux
  • .NET 6
  • Version 108.0.1

Additional context
Due to this memory-leak and a high amount of requestst we have a kubernetes hosted application that increases its memory consumtion by ~3GB in just 6 hours.
We will likly fix this issue our self by using a custom serilizer that includes caching.

@Timo-Weike Timo-Weike added the bug label Dec 28, 2022
@Timo-Weike
Copy link
Author

We will use in our project for now this custom class

public class DotNetXmlSerializerWithCache : IXmlSerializer
{
    private static readonly Dictionary<(Type, string?), XmlSerializer> cache = new();
    private static readonly ReaderWriterLockSlim cacheLock = new();

...

    /// <summary>
    /// Serialize the object as XML
    /// </summary>
    /// <param name="obj">Object to serialize</param>
    /// <returns>XML as string</returns>
    public string Serialize(object obj)
    {
        var ns = new XmlSerializerNamespaces();

        ns.Add(string.Empty, this.Namespace);

        var serializer = GetXmlSerializer(obj.GetType(), this.RootElement);

        var writer = new EncodingStringWriter(this.Encoding);

        serializer.Serialize(writer, obj, ns);

        return writer.ToString();
    }

    private static XmlSerializer GetXmlSerializer(Type type, string? rootElement)
    {
        // We use a read write lock so that the read of an already created instance
        // is fast.
        // We don't use ConcurrentDictionary because the GetOrAdd method does
        // not guarantee that only one instance would be created.

        XmlSerializer? serializer = null;
        var key = (type, rootElement);

        cacheLock.EnterReadLock();

        try
        {
            if (cache.ContainsKey(key))
            {
                serializer = cache[key];
            }
        }
        finally
        {
            cacheLock.ExitReadLock();
        }

        if (serializer != null)
        {
            return serializer;
        }

        cacheLock.EnterWriteLock();

        try
        {
            // check again for a cached instance, because between the EnterWriteLock
            // and the last check, some other thread could have added an instance
            if (!cache.ContainsKey(key))
            {
                var root = rootElement == null ? null : new XmlRootAttribute(rootElement);

                cache[key] = new XmlSerializer(type, root);
            }

            serializer = cache[key];
        }
        finally
        {
            cacheLock.ExitWriteLock();
        }

        return serializer;
    }
}

which is a copy of DotNetXmlSerializer but with caching.

@Timo-Weike
Copy link
Author

Can please someone from the Maintainers respond. Or should I just create a PR for the proposed fix?

@alexeyzimarev
Copy link
Member

Please submit a PR

alexeyzimarev added a commit that referenced this issue Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants