Skip to content
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

Invocations collection improvements #628

Merged
merged 10 commits into from
Jun 10, 2018
108 changes: 108 additions & 0 deletions Moq.Tests/InvocationsFixture.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
using System;
using Xunit;

namespace Moq.Tests
{
public class InvocationsFixture
{
[Fact]
public void MockInvocationsAreRecorded()
{
var mock = new Mock<IComparable>();

mock.Object.CompareTo(new object());

Assert.Equal(1, mock.Invocations.Count);
}

[Fact]
public void MockInvocationsIncludeInvokedMethod()
{
var mock = new Mock<IComparable>();

mock.Object.CompareTo(new object());

var invocation = mock.Invocations[0];

var expectedMethod = typeof(IComparable).GetMethod(nameof(mock.Object.CompareTo));

Assert.Equal(expectedMethod, invocation.Method);
}

[Fact]
public void MockInvocationsIncludeArguments()
{
var mock = new Mock<IComparable>();

var obj = new object();

mock.Object.CompareTo(obj);

var invocation = mock.Invocations[0];

var expectedArguments = new[] {obj};

Assert.Equal(expectedArguments, invocation.Arguments);
}

[Fact]
public void MockInvocationsCanBeEnumerated()
{
var mock = new Mock<IComparable>();

mock.Object.CompareTo(-1);
mock.Object.CompareTo(0);
mock.Object.CompareTo(1);

var count = 0;

using (var enumerator = mock.Invocations.GetEnumerator())
{
while (enumerator.MoveNext())
{
Assert.NotNull(enumerator.Current);

count++;
}
}

Assert.Equal(3, count);
}

[Fact]
public void MockInvocationsCanBeCleared()
{
var mock = new Mock<IComparable>();

mock.Object.CompareTo(new object());

mock.ResetCalls();

Assert.Equal(0, mock.Invocations.Count);
}

[Fact]
public void MockInvocationsCanBeRetrievedByIndex()
{
var mock = new Mock<IComparable>();

mock.Object.CompareTo(-1);
mock.Object.CompareTo(0);
mock.Object.CompareTo(1);

var invocation = mock.Invocations[1];

var arg = invocation.Arguments[0];

Assert.Equal(0, arg);
}

[Fact]
public void MockInvocationsIndexerThrowsIndexOutOfRangeWhenCollectionIsEmpty()
{
var mock = new Mock<IComparable>();

Assert.Throws<IndexOutOfRangeException>(() => mock.Invocations[0]);
}
}
}
92 changes: 69 additions & 23 deletions Source/InvocationCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,20 +47,20 @@ namespace Moq
{
internal sealed class InvocationCollection : IReadOnlyList<IReadOnlyInvocation>
{
private List<Invocation> invocations;
private Invocation[] invocations;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@stakx stakx Jun 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@stakx stakx Jun 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@stakx stakx Jun 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


public InvocationCollection()
{
this.invocations = new List<Invocation>();
}
private int capacity = 0;
private int count = 0;

private readonly object invocationsLock = new object();

public int Count
{
get
{
lock (this.invocations)
lock (this.invocationsLock)
{
return this.invocations.Count;
return count;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
Expand All @@ -69,55 +69,101 @@ public int Count
{
get
{
lock (this.invocations)
lock (this.invocationsLock)
{
if (this.count <= index || index < 0)
{
throw new IndexOutOfRangeException();
}

return this.invocations[index];
}
}
}

public void Add(Invocation invocation)
{
lock (this.invocations)
lock (this.invocationsLock)
{
this.invocations.Add(invocation);
}
}
if (this.count == this.capacity)
{
var targetCapacity = this.capacity == 0 ? 4 : (this.capacity * 2);
Array.Resize(ref this.invocations, targetCapacity);
this.capacity = targetCapacity;
}

public bool Any()
{
return this.invocations.Count > 0;
this.invocations[this.count] = invocation;
this.count++;
}
}

public void Clear()
{
lock (this.invocations)
lock (this.invocationsLock)
{
this.invocations.Clear();
// Replace the collection so readers with a reference to the old collection aren't interrupted
this.invocations = null;
this.count = 0;
this.capacity = 0;
}
}

public Invocation[] ToArray()
{
lock (this.invocations)
lock (this.invocationsLock)
{
return this.invocations.ToArray();
if (this.count == 0)
{
return new Invocation[0];
}

var result = new Invocation[this.count];

Array.Copy(this.invocations, result, this.count);

return result;
}
}

public Invocation[] ToArray(Func<Invocation, bool> predicate)
{
lock (this.invocations)
lock (this.invocationsLock)
{
return this.invocations.Where(predicate).ToArray();
if (this.count == 0)
{
return new Invocation[0];
}

var result = new List<Invocation>(this.count);

for (var i = 0; i < this.count; i++)
{
var invocation = this.invocations[i];
if (predicate(invocation))
{
result.Add(invocation);
}
}

return result.ToArray();
}
}

public IEnumerator<IReadOnlyInvocation> GetEnumerator()
{
lock (this.invocations)
// Take local copies of collection and count so they are isolated from changes by other threads.
Invocation[] collection;
int count;

lock (this.invocationsLock)
{
collection = this.invocations;
count = this.count;
}

for (var i = 0; i < count; i++)
{
return this.invocations.ToList().GetEnumerator();
yield return collection[i];
}
}

Expand Down