Skip to content

Assert.That is blocking and might lead to deadlock when used with WCF. #3432

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

Closed
MikiKaufhold opened this issue Nov 21, 2019 · 4 comments
Closed

Comments

@MikiKaufhold
Copy link

Problem 1)

Suppose you are asserting an asynchronous WCF service call - for the sake of simple example just its completion (for complete implementation see NUnitUsingWCFClientDeadlockReproducer.cs.txt)

Assert.That(async () =>
{
  await AsyncCall();
  return true;
}, Is.True);

In certain cases this lead to deadlock…

Analysis 1)

There is a great article where Alois explains a case of similar WCF deadlock

The problem is that WCF runs asynchronous method completions on the WCF channel dispatcher which seems to be single threaded just like a UI application with a message pump.

 A sync/async mixture of remoted methods will likely cause deadlocks

Interestingly, our WCF service has only Async methods…
However, Assert.That ends up with a blocking Task.Wait() call and (likely) causes the deadlock.

Solution 1)

To resolve this we have introduced an async variant for Assert.That using DelayedConstraint that looks like this:
public static Task DelayedAsync<T>(Func<Task<T>> actualValueDelegate, IResolveConstraint expression, int delayInMilliseconds)
(for complete implementation see DelayedAsync.cs.txt)

Problem 2) - Considering upgrade of NUnit from 2.6.6 -> 3.11

We are running into the issue generally known as "Catching AssertionException fails tests" #2758 but also mentioned in #2043, #2040, #2007.

Analysis 2)

As suggested in those threads,
using (new NUnit.Framework.Internal.TestExecutionContext.IsolatedContext()) in the most inner DelayedAsync call resolves the issue.

However, since catching AssertionExceptions is not recommended practise and TestExecutionContext is Internal, I am hesitant to adopt this as a final solution.

Instead I would wish for a native awaitable Assert.That that would implement async all the way pattern as recommended by Stephen Cleary.

Looking forward to your feedback and suggestions.

@jnm2
Copy link
Contributor

jnm2 commented May 20, 2020

We mitigate deadlocks for known synchronization contexts, currently just Windows Forms and WPF message loops. #2917 tracks making this extensible.

Does WCF have a synchronization context and a MessageLoop.Run() and MessageLoop.Exit() equivalent? We might be able to detect it and do everything in the box.

Instead I would wish for a native awaitable Assert.That that would implement async all the way pattern as recommended by Stephen Cleary.

I'm with you 110%. #2843 tracks this.

@OsirisTerje
Copy link
Member

@uecasm @MikiKaufhold Can you guys confirm https://www.myget.org/feed/nunit/package/nuget/NUnit/4.0.0-dev-07733 is working for you?

@OsirisTerje OsirisTerje added this to the 4.0 milestone Apr 6, 2023
@siprbaum
Copy link

siprbaum commented Apr 7, 2023

Cc @z002Holpp, who I know was also affected by this.

@OsirisTerje
Copy link
Member

This has not been confirmed by any of the reporters, so I'll close this issue with the assumption it works. The PR has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants