Could locking an enumerable potentially cause multiple enumeration?

Question

I think ReSharper is lying to me.

I have this extension method that (hopefully) returns an xor of two enumerations:

public static IEnumerable<T> Xor<T>(this IEnumerable<T> first, IEnumerable<T> second)
{
    lock (first)
    {
        lock (second)
        {
            var firstAsList = first.ToList();
            var secondAsList = second.ToList();

            return firstAsList.Except(secondAsList).Union(secondAsList.Except(firstAsList));
        }
    }
}

ReSharper thinks I'm performing a multiple enumeration of an IEnumerable, as you can see, on both the arguments. If I remove the locks, then it's satisfied that I'm not.

Is ReSharper right or wrong? I believe it's wrong.

---

edit: I do realize that I'm enumerating the lists multiple times, but ReSharper is saying I'm enumerating over the original arguments multiple times, which I don't think is true. I'm enumerating both arguments once into a list so I may then perform the actual set manipulation, but as I see it, I'm not actually iterating over the arguments passed multiple times.

For example, if the passed arguments are actually query results, my belief is this method won't cause a storm of queries to be executed by the set manipulation. I do understand what ReSharper means by warning of multiple enumeration: if the enumerables passed are heavy to generate, then if they're enumerated multiple times, then performing multiple enumerations on them will be much slower.

Also, removing the locks definitely makes ReSharper happier:

Answer 1

You are indeed enumerating both of the lists multiple times. You are not enumerating the enumerables passed as parameters multiple times. Both lists are enumerated once for each call to Except. The call to Union is not enumerating either sequence an additional time, but rather is enumerating the results of the two calls to Except.

Of course, iterating a List multiple times in a context like this isn't really a problem; there aren't negative consequences to iterating an unchanging list multiple times.

The lock statements have nothing whatsoever to do with enumeration of the sequences. Locking on an IEnumerable does not iterate it. Of course, locking on two objects like this, specifically two objects that are not limited in scope to this section of code, is very dangerous. It's quite possible to end up deadlocking the program with locks used in this manor if code elsewhere (such as another invocation of this method) ends up taking locks on the same objects in the opposite order).

Answer 2

This is a bit of a funny one.

First things first: as you've correctly identified, R# is raising this inspection not against the multiple usages of the Lists - there is of course nothing to worry about in multiply enumerating a List - but against (what R' sees as multiple usages of) the IEnumerable arguments. I'm presuming you already know why this would be potentially bad, so I'll skip that.

---

Now to the question of whether R# is right to complain here. To quote the C# spec,

A lock statement of the form

lock (x) ...

where x is an expression of a reference-type, is precisely equivalent to

System.Threading.Monitor.Enter(x);
try {
  ...
}
finally {
  System.Threading.Monitor.Exit(x);
}

except that x is only evaluated once.

(I've put in this emphasis because I like this wording; it avoids debates (that I'm definitely not qualified to enter) about whether this is "syntatic sugar" or not.)

Taking a minimal example which produces this R# inspection:

private static void Method(IEnumerable<int> enumerable)
{
    lock (enumerable)
    {
        var list = enumerable.ToList();
    }
}

and replacing it by what I think is the precisely equivalent version as mandated by the spec:

private static void Method(IEnumerable<int> enumerable)
{
    var x = enumerable;
    System.Threading.Monitor.Enter(x);
    try
    {
        var list = enumerable.ToList();
    }
    finally
    {
        System.Threading.Monitor.Exit(x);
    }
}

also produces the inspection.

The question then is: is R# right to produce this inspection? And this is where I think we get into a grey area. When I pass the following enumerable to either of these methods:

static IEnumerable<int> MyEnumerable()
{
    Console.WriteLine("Enumerable enumerated");
    yield return 1;
    yield return 2;
}

it is not multiply enumerated, which would suggest that R# is wrong to warn here; however, I can't actually find anything in documentation that guarantees this behaviour of either lock or Monitor.Enter. So for me it's not quite as clear-cut as this R# bug I reported, where use of GetType flagged this inspection; but nonetheless I'd guess you're safe.

If you raise this on the R# bug tracker, you can get JetBrains' finest looking at a) whether this behaviour is indeed guaranteed. and b) whether R# can be adjusted to either not warn, or provide a justification for warning.

---

That said, of course, using locking here probably isn't actually achieving what you want to achieve, as stated in other answers and comments...