Invocations collection improvements #628

Code-Grump · 2018-06-10T12:35:46Z

Add tests for Mock.Invocations property.
Improved InvocationCollection to no longer allocate a copy of the collection on enumeration.

stakx

Hi there, thanks for the new PR. Just a handful of questions and smallish requests.

stakx · 2018-06-10T13:08:23Z

Moq.Tests/InvocationsFixture.cs

+		    mock.Object.CompareTo(0);
+		    mock.Object.CompareTo(1);
+
+		    var count = mock.Invocations.Count();


I understand what you're doing here, but I wonder if this could be implemented more clearly. What if LINQ's .Count() operator has special handling for IReadOnlyList<> (as requested by e.g. https://github.com/dotnet/corefx/issues/24773)? Then this possibly wouldn't enumerate at all! If the test is about enumeration, it would be good to have the enumeration more visible.

Fair point. I'll break it out into a foreach to be certain.

Actually to be 100% certain, I'll call the enumerator manually. I know foreach has some special behaviours for lists.

stakx · 2018-06-10T13:09:17Z

Moq.Tests/InvocationsFixture.cs

+			Assert.Equal(mock.Invocations.Count, count);
+	    }
+	}
+}


Could you perhaps add an additional test that verifies that mock.ResetCalls() empties mock.Invocations?

Sounds like a good idea to me.

stakx · 2018-06-10T13:10:12Z

Source/InvocationCollection.cs

@@ -47,20 +47,20 @@ namespace Moq
 {
 	internal sealed class InvocationCollection : IReadOnlyList<IReadOnlyInvocation>
 	{
-		private List<Invocation> invocations;
+		private Invocation[] invocations;


I suppose I might be missing something obvious here, but I'd really like to avoid us reinventing the wheel and programming another version of List<>. Why is this necessary?

If we continue to use List<T> you also have to use List<T>.Enumerator which isn't going to behave the way we want in concurrent-execution scenarios. Arrays, being primitives, have a much simpler access model, so we can know and take advantage of what operations are safe to do concurrently and which are not.

Wouldn't we only end up with List<T>.Enumerator if we used LINQ or a foreach loop? What if we went for a simple for loop that accesses the list by index in the range 0..n where n is the list's Count at the moment when enumeration starts? Count can only increase. If a Clear is performed, that won't have to affect already-started enumerations since they continue seeing an old, detached list.

I did consider it, but it has the same problem. When you call List<T>.Add(T), it's not concurrency-safe with its indexer. A path exists where you wind up in an unknown state, like if the underlying array has to be resized. We could try to work out how the implementation of List<T> works and try to handle these edge-cases, but I think it's considerably easier to just use an array. The only method with any real complexity is our Add(Invocation) and most of that is delegated to the fantastic Array.Resize method.

I can't see the problem yet. Due to the locks in InvocationCollection, only one thread can ever access the list at a time. If an Add causes a resize, then that resize happens inside the lock and noone else can read the list. The indexer read access also happens inside a lock, so while that happens, noone can modify the list.

Sorry if the following might be a little naïve—I'm not sure right now how the C# compiler transforms lock blocks when yield return gets involved, a single lock around the whole method body might actually suffice:

IEnumerator<IReadOnlyInvocation> GetEnumerator() { List<Invocation> invocations; int count; lock (this.invocationsLock) { invocations = this.invocations; // copying the list reference into a local count = invocations.Count; // means that a `Clear` shouldn't affect us } for (int i = 0; i < count; ++i) // the list is guaranteed to have at least count items { // even if modified between two iterations lock (this.invocationsLock) { yield return invocations[i]; // no modification can happen here, inside a lock } } }

A lock around the for loop would actually make it so the enumerator would acquire the collection until all elements had been iterated over. What you have should acquire a lock to the collection every time it yields an item, then release the lock.

I do like how much less code there is here. I also like that my implementation only acquires a lock once, versus once-per-item + 1 times. I could go either way.

Perhaps there is a middle way. I feel it should be possible to let the compiler generate the enumerator class, which would help reduce the amount of additional code. I'll look into this more closely a little later.

Added an implementation using yield return. It should be functionally identical to the original, except it doesn't support IEnumerator.Reset().

stakx · 2018-06-10T13:13:31Z

Source/InvocationCollection.cs

+					return new Invocation[0];
+				}
+
+				return this.invocations.Take(this.count).Where(predicate).ToArray();


I wonder if you could use Array.Copy or List<T>.CopyTo instead of a LINQ query?

I think it could be re-written to use those calls, but the predicate makes it a little harder. I'm not sure if there's a benefit to do so.

Less unnecessary overhead, basically. This class is pretty low-level, I usually try to avoid LINQ in such places.

(Sorry, I was an idiot above. I was looking at the wrong lines of code when I wrote the above, I completely overlooked the predicate.)

stakx · 2018-06-10T13:16:01Z

Source/InvocationCollection.cs

 			{
-				return this.invocations.ToList().GetEnumerator();
+				return new Enumerator(this.invocations, this.count);


I'm wondering, why not the LINQ range-based approach you suggested earlier?

Take creates an enumerator which operates on the underlying collection. If the collection is modified on another thread whilst working with this enumerator, at best you'll get an InvalidOperationException and at worse will end up in an unknown state. Previously, copying was used to prevent modification by another thread. This custom Enumerator instead takes advantage of append-only nature of the collection and allows enumeration over the known state of the collection even when new items are appended to the collection.

OK, fair enough, the LINQ query wouldn't work. But see my comment above regarding a plain indexing for loop.

stakx

Thanks for the changes so far, this is basically good to go now. I noticed a few more final details, I'd be happy if you could check them out.

stakx · 2018-06-10T16:49:53Z

Source/InvocationCollection.cs

@@ -69,7 +69,7 @@ public int Count
 		{
 			get
 			{
-				lock (this.invocations)
+				lock (this.invocationsLock)
 				{
 					return this.invocations[index];


Could you please add an explicit index-in-range check here / throw IndexOutOfRangeException otherwise?

Won't we already just get one from the array anyway? Did you want to add some specific detail?

There may be no array; you set the field to null in Clear, so the indexer might leak a NullReferenceException. I believe that an out-of-range index access should reliably throw IndexOutOfRangeException instead of leaking implementation details.

Ah, I hadn't thought of this scenario. I wouldn't want a NullReferenceException either.

stakx · 2018-06-10T16:53:50Z

Source/InvocationCollection.cs

 				{
-					return this.invocations.Count;
+					return count;
 				}


I believe this lock is superfluous... please remove. Reading an int is an atomic operation, there's no danger of struct tearing or anything of the sort. If there is a concurrent mutation happening during a read, it doesn't really matter whether external code sees the original or updated count.

(In fact, we could question the whole point of having a Count property in a concurrent collection, since user code isn't supposed to do non-atomic accesses like if (invocations.Count > 0) firstInvocation = invocations[0];... but having Count can still be useful in single-threaded scenarios.)

It's not there to avoid a torn read, but to protect against reordering optimisations and variable caching. If you want to go down that avenue, it might require marking the backing field as volatile, or otherwise setting up a memory barrier. I don't advise pulling on this thread unless you think it's very important.

I didn't know reordering happened across different methods. But yeah, this isn't super-important. Let's leave it as is, then.

stakx · 2018-06-10T16:58:48Z

Source/InvocationCollection.cs

+			lock (this.invocationsLock)
+			{
+				return this.count > 0;
+			}


This lock isn't necessary (for the same reason mentioned at Count), please remove.

(In fact, now that we're implementing IEnumerable, I wonder if this method is still needed at all: We're only using it in a single place—VerifyNoOtherCalls—which in typical user code is perhaps run on the order of once per unit test method, so we could probably afford just letting Moq use LINQ's .Any() operator and going through .GetEnumerator(). What do you think? Feel free to remove just the lock, or the whole method.)

I think LINQ's Any() might just call Count in this case. It's probably fine to remove the method.

That's fine with me, let's remove it.

stakx · 2018-06-10T16:59:53Z

Source/InvocationCollection.cs

+					}
+				}
+
+				Array.Resize(ref result, resultSize);


Haha, I know I previously said that I tend to avoid LINQ queries in low-level places such as this class... but please feel free to just use a List<> with an initial capacity of this.count, then returning .ToArray() in the end. It's a common pattern that would save you some lines of code at only very little additional overhead. (It's your call, though.)

stakx · 2018-06-10T18:01:15Z

Source/InvocationCollection.cs

+				if (this.invocations == null)
+				{
+					throw new IndexOutOfRangeException();
+				}


Unfortunately, this still isn't sufficient. The array can have a capacity that exceeds 'count' so you are not going to get an out-of-range exception in some cases. I suggest you just add a straightforward check for 0 <= index && index < this.count instead. ;-)

stakx · 2018-06-10T18:02:22Z

Source/InvocationCollection.cs

+			lock (this.invocationsLock)
+			{
+				return this.count > 0;
+			}


That's fine with me, let's remove it.

stakx · 2018-06-10T19:01:33Z

Looks like you have made all the requested changes, so let's merge this. Thanks for your great work!

Code-Grump added 2 commits June 10, 2018 12:44

Add tests for invocations collection

25895f1

Add thread-safety improvements to InvocationCollection

11f1027

Code-Grump mentioned this pull request Jun 10, 2018

Expose invocations #627

Merged

stakx suggested changes Jun 10, 2018

View reviewed changes

Code-Grump added 3 commits June 10, 2018 14:53

Improve enumeration test and add clear test

5738a90

Remove Linq calls from ToArray methods

8dc2ce3

Switch custom Enumerator to using yield return

61cdb4d

stakx suggested changes Jun 10, 2018

View reviewed changes

Code-Grump added 3 commits June 10, 2018 18:42

Add tests and check for indexer

0dbb1bb

Remove obsolete Any method

c9dd227

Fix broken indexer check

ef18220

stakx suggested changes Jun 10, 2018

View reviewed changes

Code-Grump added 2 commits June 10, 2018 19:13

Fix index check

108688d

Simplified ToArray-with-predicate overload

aebd6db

stakx approved these changes Jun 10, 2018

View reviewed changes

stakx merged commit f44e690 into devlooped:master Jun 10, 2018

Invocations collection improvements #628

Invocations collection improvements #628

Conversation

Code-Grump commented Jun 10, 2018

stakx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

stakx Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Code-Grump Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

stakx Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

stakx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakx commented Jun 10, 2018

stakx Jun 10, 2018 •

edited

Loading

stakx Jun 10, 2018 •

edited

Loading

stakx Jun 10, 2018 •

edited

Loading

Code-Grump Jun 10, 2018 •

edited

Loading

stakx Jun 10, 2018 •

edited

Loading

stakx Jun 10, 2018 •

edited

Loading

stakx Jun 10, 2018 •

edited

Loading

stakx Jun 10, 2018 •

edited

Loading