Usage of ReturnsExtensions.ThrowsAsync() can cause UnobservedTaskException

[snrnats]

Usage of ReturnsExtensions.ThrowsAsync() will cause UnobservedTaskException if:

1. Set up a mock method with ReturnsExtensions.ThrowsAsync()

            var mock = new Mock<A>();
            mock.Setup(a => a.Foo()).ThrowsAsync(new ArgumentException());

2. Don't call the method you set up

Scenario where it can cause troubles is when you want to check that your code doesn't throw any UnobservedExceptions. It will looks like this
Sample project MoqThrowsAsync.zip or code:

    [TestClass]
    public class UnitTest1
    {
        private UnobservedTaskExceptionEventArgs _unobservedEventArgs;

        [TestInitialize]
        public void Init()
        {
            TaskScheduler.UnobservedTaskException += OnUnobservedTaskException;
        }

        private void OnUnobservedTaskException(object sender, UnobservedTaskExceptionEventArgs e)
        {
            _unobservedEventArgs = e;
        }

        [TestCleanup]
        public void Clenup()
        {
            GC.Collect();
            GC.WaitForPendingFinalizers();
            TaskScheduler.UnobservedTaskException -= OnUnobservedTaskException;
            if (_unobservedEventArgs != null && !_unobservedEventArgs.Observed)
            {
                throw _unobservedEventArgs.Exception;
            }
        }

        [TestMethod]
        public void TestMethod1()
        {
            var mock = new Mock<A>();
            mock.Setup(a => a.Foo()).ThrowsAsync(new ArgumentException());
        }
    }

    public interface A
    {
        Task Foo();
    }

[stakx]

Hi @snrnats. Thanks for reporting. I'll try to look into this during the next few days! Right now, my best first guess is that in this line (and other similar ones):

https://github.com/moq/moq4/blob/b7b662774f27c46f32e715a75ddf180186cf8502/Source/ReturnsExtensions.cs#L90

we might instead have to use a lambda (.Returns(() => tcs.Task)) so that the faulted task only gets created when the setup is actually invoked. Feel free to look into this, if you find the time.

[stakx]

@snrnats - TL;DR: Would you like to submit a PR to fix this problem? You can find instructions on how to do so below.

Cause of this problem:

My above guess was on the right track. This issue is indeed caused by the faulted task not getting created only on demand / in a lazy fashion. Here's what the ThrowsAsync method's body currently looks like (there are several other similar overloads, let's just look at the one you're actually using):

var tcs = new TaskCompletionSource<bool>();
tcs.SetException(exception);
return mock.Returns(tcs.Task);

Let's try to access tcs.Task in a deferred manner:

var tcs = new TaskCompletionSource<bool>();
tcs.SetException(exception);
return mock.Returns(() =>
{
	return tcs.Task;
});

That won't help. Seems that tcs is building the faulted task even before tcs.Task is accessed. So let's fault the task lazily, too:

var tcs = new TaskCompletionSource<bool>();
return mock.Returns(() =>
{
	tcs.SetException(exception);
	return tcs.Task;
});

This fixes the problem you've reported!

Since we've gone that far already, there's no point in creating tcs early (it might never be needed if the setup is not invoked), so we end up with this:

return mock.Returns(() =>
{
	var tcs = new TaskCompletionSource<bool>();
	tcs.SetException(exception);
	return tcs.Task;
});

If you'd like to submit a PR for fixing this:

• Please quickly let me know here if you'd like to submit a PR. The following would be required:

• Add your test to Moq.Tests/Regressions/IssueReportsFixture.cs (note that the file is roughly sorted by issue origin & number, so add the following just before the comment line that reads, // Old @ Google code):

  #region 592
  
  public class Issue592
  {
      // Your complete test code, including any types, goes here.
      // Note that Moq uses xUnit, so `[TestInitialize]` = ctor, `[TestMethod]` = `[Fact]`,
      // and `[TestCleanup]` = IDisposable implementation (IIRC).
  }
  
  #endregion

• Fix all ThrowsAsync methods in Source/ReturnsExtensions.cs as described above. Note that any Guard statements (argument validation) you encounter should not be moved inside the lambda.

• Update the changelog by inserting the following about the ## 4.8.2 (...) heading:

  ## Unreleased
  
  #### Fixed
  
  * <brief description of the issue you've fixed> (@snrnats, #<PR number>)
  

If I don't hear from you in the next 10 days or so, I'll apply the above fix.

Thank you! :-)

P.S.:

The fix I've described above has one side effect: whereas until now, ThrowsAsync would cause a setup to always return the same faulted task, now a different faulted task would be returned for each setup invocation. I am not sure whether this could be a breaking change. If so, to prevent this, we might have to do something like the following (just a rough idea):

var task = new Lazy<Task>(() =>
{
	var tcs = new TaskCompletionSource<bool>();
	tcs.SetException(exception);
	return tcs.Task;
});
return mock.Returns(() => task.Value);

[snrnats]

Hi @stakx thank you for clarifying the root of the problem
I will create a PR.
I agree with your solution to use deferred method invocation.

Here are my thoughts about Lazy implementation. In real applications both scenarios are possible, that method returns a new task or the same task. For now we should keep the original behavior that returns the same task. In future I would create a parameter to allow different behavior if somebody requested it.

[snrnats]

It appears that SequenceExtensions.ThrowsAsync has the same issue. However solution isn't straightforward because ISetupSequentialResult.Returns doesn't have overload version that accepts delegate.

[stakx]

@snrnats - I think it'd be good to also fix SequenceExtensions.ThrowsAsync while we're at it. (@kzu: attention, minor API change:) Do you see any problems caused by adding the required Returns(Func<TResult>) overload to ISetupSequentialResult<TResult>?

https://github.com/moq/moq4/blob/b7b662774f27c46f32e715a75ddf180186cf8502/Source/Language/ISetupSequentialResult.cs#L16-L19

I can think of only one thing (and it's only a vague feeling; I haven't thought it through at this point, so it might not even be necessary at all): There might be some cases where TResult is some Func<>, and the compiler might end up picking the wrong method overload in user code calling that method. In order to not break code that was written before the new overload turned a single method into a method group, there'd possibly have to be a check in the new overload's implementation (in class SetupSequencePhrase<TResult> in Language/Flow/SetupSequencePhrase.cs) checking whether to execute the given Func to fetch the result, or whether the Func itself is the result. Something similar was done here:

https://github.com/moq/moq4/blob/b7b662774f27c46f32e715a75ddf180186cf8502/Source/MethodCallReturn.cs#L92-L108

Like I said, this might not be necessary, but perhaps keep this in mind.

[stakx]

Oops, this got accidentally closed due to the "resolve" keyword in #594's description:

They are needed to resolve #592

Reopening...

@snrnats, good work so far. Thank you! 👍

[stakx]

@snrnats - I'm reviewing your PR right now, and while it looks solid—I have only a few minor change requests—, it has made me realise that it might be good to think a little bit longer about consistency:

What's the difference between setup.Returns(value) (such as setup.Returns(42)) and setup.Returns(valueFunc) (such as setup.Returns(() => 42)? The former configures a setup to return the same value on each invocation, while the latter causes the setup to evaluate the given function on each invocation.

If we wanted good consistency, wouldn't this distinction be transferable to ReturnsAsync? That is, you'd have setup.ReturnsAsync(42) (which would always return the same completed task), and setup.ReturnsAsync(() => 42) (which would return a different completed task on each invocation)?

To be honest, it's entirely possible that this would take consistency too far and that most people just wouldn't intuitively understand these fine distinctions; and, like you say, this API might simply not be implementable for the method overloads that allow you to specify a delay.

So while I think it's probably better to keep the API simple (as it is now, without the value / valueFunc distinction), I'd value your opinion on my above thoughts, nevertheless.

[snrnats]

Here are my thoughts about async:
Mocking library should mimic the most conventional practices. When we say that Async method returns 42 it usually means:

        async Task<int> FooAsync()
	{
                //...
		return 42;
	}

        //or

	Task<int> Foo()
	{
		return Task.FromResult(42);
	}

In both cases we are getting a new task each time we call the method.

On the other side, sometimes we reuse the task. I am aware of at least one case when we want to mimic the behavior that keeps returning the same task:
If we know that a task needs to be executed only once. Then to reduce memory allocation, we cache the task with a result, instead of caching the result itself.

        private Task<int> _fooTask;

	private async Task<int> FooAsync()
	{
		return 42;
	}

	public Task<int> FooTask()
	{
		return _fooTask ?? FooAsync();
	}

I think that this case should be mocked in a way when you explicitly say to use the same task. Then it will mimic the same behavior as in the implementation details. It can be achieved by using regular Returns()
setup.Returns(Task.FromResult(...))