Is there memory Leak in Serializer

Question

In the below code, is there any part in the code which can cause memory leak? Can XmlSerializer cause memory leak?

public static string Serializer<T>(T tag, Encoding encoding = null)
    {
        if (encoding == null)
            encoding = Encoding.UTF8;

        MemoryStream memoryStream = null;
        try
        {
            StringBuilder xmlBuilder = new StringBuilder();
            memoryStream = new MemoryStream();
            using (StreamWriter streamWriter = new StreamWriter(memoryStream, encoding))
            {
                XmlSerializer serializer = new XmlSerializer(tag.GetType());
                serializer.Serialize(streamWriter, tag);

                using (StreamReader streamREader = new StreamReader(memoryStream, encoding))
                {
                    memoryStream.Position = 0;
                    xmlBuilder.Append(streamREader.ReadToEnd());
                }
            }
            return xmlBuilder.ToString();
        }
        catch (Exception ex)
        {
            logger.Error("{0} {1}", "Serializer failed ", ex);
            return null;
        }
    }

Any Ideas.

@bommelding: `MemoryStream` is unusual in that regard, yes, and nothing really prevents you from doing so, but I also see no point at all. First, it's best practice to dispose locally constructed disposable instances using the `using` statement. Most static code analyzers will complain about not disposing the object, and you have one more thing to explain during code reviews. Second, accessing members of a disposed object should be almost always avoided. So I would prefer `using (var ms = MemoryStream()) { x = ms.ToArray(); }` any day of the week, if I really need to return the buffer. — vgru, Oct 30 '18 at 21:06
@groo - that is drifting away from the point here, which was the false assumption like in the answer below. See [here](https://stackoverflow.com/a/52988480/9695604) why the ToArray() should come _after_ the `using`. — bommelding, Oct 31 '18 at 07:41
@JerinSebastian - some useful comments were deleted here, did you see the one about the generated assemblies? You should respond to that. Currently your question is unclear. — bommelding, Oct 31 '18 at 07:45
@bommelding: your linked answer doesn't make sense: disposing a `MemoryStream` does [absolutely nothing to the data](https://referencesource.microsoft.com/#mscorlib/system/io/memorystream.cs,147). Perhaps the OP's `LZ4Stream` does some flushing on dispose, but claiming that `ToArray()` must come after disposing `MemoryStream` is incorrect. — vgru, Nov 02 '18 at 11:37
@HenkHolterman This is not my snippet, but I'd still like to hear a reason for disposing the `MemoryStream` before getting the array. The `Dispose` method simply does nothing related to the array. Giving advice to a novice programmer (presumably) that you should access an instance after disposing is, IMHO, a poor practice. — vgru, Nov 02 '18 at 19:09
@HenkHolterman: [disposing a `MemoryStream` does absolutely nothing to the data](https://referencesource.microsoft.com/#mscorlib/system/io/memorystream.cs,147). The only thing that happens is that future calls to most methods will throw an `ObjectDisposedException`. — vgru, Nov 03 '18 at 22:08
@Groo - it certainly isn't incorrect but maybe a matter of taste. The linked answer did help someone out and I've seen that pattern a lot. You could take the array inside the `using (memstream)` but it has to be outside the `using(streamDecorator)`. — bommelding, Nov 05 '18 at 07:49

score -2 · Accepted Answer · answered Oct 30 '18 at 14:19

-2

You're not disposing the "memoryStream" you declare on 6.

And something else: you won't see the memory gap regained until the GC passes... you can check that using GC.Collect

answered Oct 30 '18 at 14:19

Leonardo

10,737
10
62
155

And it is important to note that manually forcing the GC to do its job can be problematic/cause minor performance issues. In 99.99999% of cases, waiting for it to determine it needs to run by itself is fine. – krillgar Oct 30 '18 at 14:22
1

yes! correct! you should only do that to verify that there's no leak, and even there, thats not a 100% safe approach! – Leonardo Oct 30 '18 at 14:25
If you need to manually GC, you most likely have other issues. (Like not disposing of `memoryStream`.) – krillgar Oct 30 '18 at 14:54
1

MemeoryStream.Dispose() disposes nothing. It's a dud, imposed by the base class Stream. It is not related to any memory issues the OP has. – H H Oct 30 '18 at 17:11

Is there memory Leak in Serializer

1 Answers1