8

My understanding is that LINQ IEnumerable extensions are supposed to call Dispose on the IEnumerators produced by the IEnumerable. See this answer on SO. However, I'm not seeing that in practice. Is the other SO answer incorrect, or have I found a bug in LINQ?

Here's a minimal reproduction of the issue using Mono. It also reproduces in Dotnet Core 2.1 & 2.2.

using System;
using System.Threading;
using System.Linq;
using System.Collections.Generic;
using System.Collections;


namespace union
{
    class MyEnumerable : IEnumerable<long>
    {
        public IEnumerator<long> GetEnumerator()
        {
            return new MyEnumerator();
        }

        IEnumerator IEnumerable.GetEnumerator()
        {
            return GetEnumerator();
        }
    }

    class MyEnumerator : IEnumerator<long>
    {
        public long Current
        {
            get
            {
                return 0;
            }
        }

        object IEnumerator.Current
        {
            get
            {
                return Current;
            }
        }

        public bool MoveNext()
        {
            return false;
        }

        public void Reset()
        {
            return;
        }

        public void Dispose()
        {
            Console.WriteLine("I got disposed");
        }
    }

    class Program
    {
        static void Main(string[] args)
        {
            var enum1 = new MyEnumerable();
            var enum2 = new MyEnumerable();

            enum1.Union(enum2).Select(x => x + 1).ToList();
            Console.WriteLine("All done!");
        }
    }
}

If the Dispose was getting called you would see I got disposed twice on the console. Instead you get no I got disposeds. The Union and the Select are required to reproduce the issue.

Any C# gurus know if I've hit a bug?

Update:

I believe this is a bug, and I've filed: https://github.com/dotnet/corefx/issues/40384.

Andrew
  • 529
  • 1
  • 3
  • 12

1 Answers1

8

I tested in .NET Fiddle, which allows you to use one of three compilers, and the results confirm what you found:

Seems like Mono doesn't either.

But I think I narrowed down what's going on: It does not call Dispose() when the first call to MoveNext() returns false (i.e. there are no items in the collection).

You will see this, for example, if you replace your MoveNext() code with this, which just returns true the first time and false thereafter (i.e. simulates a collection with 1 item):

private bool happenedOnce = false;
public bool MoveNext()
{
    if (happenedOnce) return false;
    happenedOnce = true;
    return true;
}

Here is that example in Mono and .NET Core 2.2.

I guess it could be argued that it's not a bug, it's a feature. What's there to dispose if there's nothing in the collection? (Scratch that - files and such...)

But then again, it is inconsistent. For example, this will give you exactly the same results, but it will dispose, even if there is nothing in the collection:

enum1.Select(x => x + 1).Union(enum2.Select(x => x + 1)).ToList();

But all that does is force it to iterate the collection using Select() instead of Union(). So that does sound like it's a bug in the implementation of Union().

I had the time today (family is away visiting friends) so I took the time to debug the .NET Core source. I believe the issue is right here:

if (enumerator.MoveNext())
{
    SetEnumerator(enumerator);
    StoreFirst();
    return true;
}

SetEnumerator() stores the enumerator in an instance variable. You see it only gets called if MoveNext() returns true. Later, in Dispose(), it only calls Dispose() on the iterator if it's not null. But since it never got set, it is indeed null.

Contrast that with the implementation of Select(), which sets its _enumerator variable before calling MoveNext().

The .NET Framework just relies on foreach, as you can see here. Oddly enough, so does Mono, so I don't know what's up there.

Gabriel Luci
  • 38,328
  • 4
  • 55
  • 84
  • 2
    _"What's there to dispose if there's nothing in the collection?"_ -- I would argue that's up to the `IEnumerator` implementation to decide, not the caller. The caller should still always call `Dispose()`, regardless. For example, the implementation might enumerate lines in a file; even for an empty file, it has to open the file to determine that there are no lines, and the file should still be closed when enumeration is done, even if no lines are returned. Seems to me this is straight up a bug in those implementations (makes me wonder how much of .NET Core came from Mono) – Peter Duniho Aug 17 '19 at 02:03
  • Yeah, I agree this is likely a bug, and I've filed an issue. https://github.com/dotnet/corefx/issues/40384 – Andrew Aug 17 '19 at 02:52
  • Beat me to it :) I can add my findings to the issue. – Gabriel Luci Aug 17 '19 at 02:53