The implementation of AwaiterMetadata.Equals is comparing objects of different types and expecting good results. This type is used as a key in a has set, it should be corrected.

The analyzer suggests changing the name of a method from its non-async version to its async version without realizing the async method is itself.

Example:

public async task CallMainAsync(){
  // do stuff
  CallMain()
  // do stuff
}

public void CallMain(){
  // more stuff
}

Analyzer will recommend changing CallMain() into CallMainAsync() without realizing CallMainAsync() is itself and that CallMain() is not the non-async counterpart but instead a synchronous part of CallMainAsync()

Add analyzer to verify that all functionality of a class that is exposed with JTF.Run is also available via an async method.

Sample bad cases:

public void Foo(int a) {
   // ...lots of code...
   JoinableTaskFactory.Run(async delegate {
      // lots more code.
   });
   // ...lots of code...
}

public void Bar(int a) {
   JoinableTaskFactory.Run(() => BarAsync(a)); // calling an async method with less visibility than the sync version.
}

internal void BarAsync(int a) {
   // code
}

Sample good cases:

public void Foo(int a) {
   JoinableTaskFactory.Run(() => FooAsync(a));
}

public async Task FooAsync() {
   await Task.Yield();
}

Proposed Spec

We seek to drive people to expose all their async code publicly so that the benefits of asynchrony is shared with all callers. JTF.Run represents an end to the benefits of asynchronous methods and thus should only be used where necessary. For example, when implementing an interface already constrained to be synchronous. But such functionality should also be exposed directly as async without the JTF.Run wrapper. Thus the best body for a JTF.Run delegate would be to simply forward the call to the async equivalent of the method.

A simple version of this analyzer then may require that a Foo method do nothing but call FooAsync inside its JTF.Run delegate and return the result (if applicable). While this would seem to work most of the time, it would misfire when the older synchronous API contains out parameters, or otherwise must shim inputs or outputs in some way. For example consider that a legacy interface may exist that looks like this:

public interface IFoo {
   int GetName(out string name);
}

Later we realize we want to expose an async version of this. We also want to make it more user-friendly, so we return the string instead of an HRESULT:

public interface IFoo2 {
   Task<string> GetNameAsync();
}

Now the implementation should ideally be written like this:

public class Foo : IFoo, IFoo2 {
    public async Task<string> GetNameAsync() {
        await Task.Yield();
        return "Andrew";
    }

    public int GetName(out string name) {
        try {
            name = JoinableTaskFactory.Run(() => GetNameAsync());
            return 0;
        } catch (Exception ex) {
            return Marshal.GetHRForException(ex);
        }
    }
}

You can see how more code than just a direct JTF.Run is necessary for the legacy method to maintain backward compatibility even though all the functionality is exposed asynchronously. But we also observe that the entirety of this shim method could be implemented by the caller (no non-public types or members are accessed) so this is how we might detect that it's allowable.

Proposed rules for creating a diagnostic:

• Only consider public properties and methods of public types. VSSDK009 will take care of the non-public methods.
• Any method with JTF.Run in it can only invoke or access members that are also public, both inside and outside the delegate passed to JTF.Run. Any access or invocation of a non-public member represents work that cannot be done asynchronously by the caller.

Consider:

• If the public method that uses JTF.Run implements an interface (either explicitly or implicitly) then all members it accesses must also implement an interface.

Alternative spec

Whenever a non-Task returning method Name() uses JTF.Run, create a warning if a NameAsync() method does not also exist.

Pros:

• Super simple

Cons:

• Does not defend against Name() containing unique functionality that is not exposed in NameAsync().

Focus on where the name can be changed -- not on implementations that must honor the original name.

In reviewing the code for WaitWithoutInlining it occurred to me that inlined continuations of the waited Task may execute before WaitWithoutInlining returns to the caller. That might introduce deadlocks at worst, or at best just slow down the caller unnecessarily and defeats some of the point of the method's existence.

The wixlib offered by the downlevel installer only works down to VS2012 (Dev11).

With a requirement of .NET 4.5, we should be able to port it all the way down to VS2010, which some customers would find useful.

Source for the installer

The JoinableTaskContextTests.GetHangReportWithActualHang test is written in a way that suggests an exception is thrown, but it does not actually verify that the test is thrown (the test will pass whether or not it's thrown). If possible, this test should be updated to only allow one successful path.

From a build where this test failed, here is the output of the test:

    SwitchToMainThreadShouldNotLeakJoinableTaskWhenGetResultRunsLater [FAIL]
      Assert.Null() Failure
      Expected: (null)
      Actual:   Object { }
      Stack Trace:
        src\Microsoft.VisualStudio.Threading.Tests.Shared\JoinableTaskTests.cs(3141,0): at Microsoft.VisualStudio.Threading.Tests.JoinableTaskTests.SwitchToMainThreadShouldNotLeakJoinableTaskWhenGetResultRunsLater()

Here is the relevant source line

When I try to run the tests locally (via Visual Studio 2017 Test Explorer), I get the following message:

Message: System.IO.FileLoadException : Could not load file or assembly 'Microsoft.VisualStudio.Threading, Version=15.3.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. Strong name signature could not be verified. The assembly may have been tampered with, or it was delay signed but not fully signed with the correct private key. (Exception from HRESULT: 0x80131045)

I tried editing the Microsoft.VisualStudio.Threading.csproj project file to disable signing temporarily (with <SignAssembly>False</SignAssembly>, but that just changed the error message to the following:

Message: System.IO.FileLoadException : Could not load file or assembly 'Microsoft.VisualStudio.Validation, Version=15.3.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. Strong name signature could not be verified. The assembly may have been tampered with, or it was delay signed but not fully signed with the correct private key. (Exception from HRESULT: 0x80131045)

In the 15.1.9 build, we had a test fail that normally passes. It is unrelated to the code change as that code had already built in PR validation and the test passed. See also a subsequent rebuild of that commit.

  OnHangDetected_RunAsync_OnMainThread_BlamedMethodIsEntrypointNotBlockingMethod [FAIL]
      Assert.NotNull() Failure
      Stack Trace:
        src\Microsoft.VisualStudio.Threading.Tests.Shared\JoinableTaskContextNodeTests.cs(200,0): at Microsoft.VisualStudio.Threading.Tests.JoinableTaskContextNodeTests.OnHangDetected_RunAsync_OnMainThread_BlamedMethodIsEntrypointNotBlockingMethod()

Because xunit's Assert class offers both Throws and ThrowsAsync, VSTHRD103 will claim an async test method should call the async version, when of course that is only appropriate if the delegate passed to the method is itself async.

    [Fact]
    public async Task ReadByte_ReturnsMinus1AtEndOfStream()
    {
        this.bufferingStream = new MemoryStream();
        Assert.Throws<InvalidOperationException>(() => this.bufferingStream.ReadByte());
    }

The following snippet produces multiple diagnostics per line:

Task<int> FooAsync()
{
    Task t = Task.FromResult(1);
    t.GetAwaiter().GetResult(); // VSSDK001, VSSDK008, VSSDK009
    jtf.Run(async delegate { await BarAsync(); }); // VSSDK008, VSSDK009
    return Task.FromResult(1);
}

Ideally, each infraction should produce just one diagnostic.

Chess could give us a way to find concurrency bugs in the library.

The following code requires two warning suppressions for the same by-design "violation". If VSTHRD002 signals a violation, being more severe, VSTHRD102 shouldn't double up.

private static bool CopyTaskResultIfCompleted<T>(Task<T> task, IVsTaskCompletionSource taskCompletionSource)
{
    if (task.IsCanceled)
    {
        taskCompletionSource.SetCanceled();
    }
    else if (task.IsFaulted)
    {
        taskCompletionSource.SetFaulted(Marshal.GetHRForException(task.Exception));
    }
    else if (task.IsCompleted)
    {
#pragma warning disable VSTHRD102 // Implement internal logic asynchronously (We already confirmed task.IsCompleted)
#pragma warning disable VSTHRD002 // Avoid problematic synchronous waits (We already confirmed task.IsCompleted)
        taskCompletionSource.SetResult(task.Result);
#pragma warning restore VSTHRD002 // Avoid problematic synchronous waits
#pragma warning restore VSTHRD102 // Implement internal logic asynchronously
    }
    else
    {
        return false;
    }

    return true;
}

The IsMainThreadBlockedFalseWhenSyncBlockingOtherThread test doesn't wait for the background task to complete, and therefore won't actually fail if the task fails.

JoinableTask's JoinAsync and JoinAsync<T> methods have unconfigured awaits which some legacy code that calls an async method and then .Wait()'s on the result ends up deadlocking on.

It's completely arguable that the caller is buggy. But as it is legacy and calling JTF.Run for them may be a significant change, we might help them out here.

This test has been observed to fail:

    RunAsyncWithNonYieldingDelegateNestedInRunOverhead [FAIL]
      Assert.Equal() Failure
      Expected: 1
      Actual:   0
      Stack Trace:
        src\Microsoft.VisualStudio.Threading.Tests.Shared\JoinableTaskTests.cs(3008,0): at Microsoft.VisualStudio.Threading.Tests.JoinableTaskTests.RunAsyncWithNonYieldingDelegateNestedInRunOverhead()

When Foo() and FooAsync() exists, VSSDK008 creates a warning when Foo() is called from an async method, even though Foo() also returns Task and is being awaited on.

Task Foo() => null;
Task FooAsync() => null;

async Task BarAsync() {
   await Foo(); // VSSDK008 fires here, although it shouldn't.
}

The documentation as seen here does not match the actual test behavior.

One of the AsyncReaderWriterResourceLock<TMoniker, TResource> constructors appears to leave the helper field uninitialized:

https://github.com/Microsoft/vs-threading/blob/14f77875ae3a04108f0bd94ee9a1124dcf4a3073/src/Microsoft.VisualStudio.Threading.Shared/AsyncReaderWriterResourceLock.cs#L47-L50

VSSDK002 should not fire on this property getter, but it does.

using System;
using Microsoft.VisualStudio.Shell;
using Microsoft.VisualStudio.Shell.Interop;

class Test {
    int F {
        get {
            ThreadHelper.ThrowIfNotOnUIThread();
            IVsSolution sln = null;
            sln.SetProperty(1000, null);
            return 0;
        }
    }
}

In some cases the VSTHRD010 warning doesn't show with some unit test that use IVsHierarchyItem with MSTest

Repro found in AsyncPackage.GetServiceAsync, but is abstractly reproduced here:

Task Foo() {
    task.ContinueWith(t => {
        Console.WriteLine(t.Result); // misfire here
    });
}

Example

AsyncSemaphore lck;
using (lck.EnterAsync())
{
    // ...
}

Expected warning:

Using statement resource is a Task<T>, where T is disposable

Expected code fix (when enclosing method is async):

AsyncSemaphore lck;
using (await lck.EnterAsync())
{
    // ...
}

When you have code like this:

public void Foo() {
   DoInterestingStuff();
   if (someCondition) {
       ThreadHelper.ThrowIfNotOnUIThread();
       DoMoreStuff();
   }
}

It's problematic because callers usually aren't prepared for thread affinity sometimes. As a result, code that happens to not execute the conditional logic of the method will temporarily get away with it but then get stung by it later. We want to front-load discovery of such bugs as early as possible by making synchronous methods unconditionally thread affinitized. So a new analyzer should require that the thread-asserting statement appear near the top of the method, and outside of any conditional blocks.

Another common anti-pattern is this one:

Debug.Assert(ThreadHelper.CheckAccess());

This is just another form of a conditional check, except that it never fires in production code, so it is largely ineffective in library code.

VSTHRD010 should not do this because a programmer may need to place the check somewhere else without disabling all the benefits of VSTHRD010.

From @CyrusNajmabadi on May 10, 2017 0:30

From a discussion with @AArnott ,

I'm seeing super high CPU utilization in VS when we use StreamRPC to talk to our OOP server. ETW logs reveal this as the top exclusive problem:

I’m investigating some local traces that show high CPU usage in VS during OOP scenarios. The traces end up showing a large amount of time around locks and contention:

--

That’s 18% exclusive time in VS time just around these locking primitives. Looking into what calls this I can see the following:

And

Copied from original issue: microsoft/vs-streamjsonrpc#37

Consider that some private methods are used in callbacks, such that although they aren't declared with public visibility, they are still called by outside code that requires a specific method signature.

For this reason, VSSDK009 should be adjusted to not produce warnings when JTF.Run is used in this case.

I was reading through the threading rules and came across the section on debugging hangs in the JTF:

In the meantime, the most useful technique for analyzing async hangs is to attach WinDBG to the process and dump out incomplete async methods' states. This can be tedious, but we have a script in this file that you can use to make it much easier: Async hang debugging

Unfortunately the link appears to be broken. Is this page available? I couldn't find it with a quick search for .md files in this repo.

This report from John Hart:

public static T WaitAndGetResult<T>(this Task<T> task, CancellationToken cancellationToken)
{
   return ThreadHelper.JoinableTaskFactory.Run(() => task.WithCancellation(cancellationToken));
}

This test has been observed to fail:

    RunAsyncWithYieldingDelegateNestedInRunOverhead [FAIL]
      Assert.Equal() Failure
      Expected: 1
      Actual:   0
      Stack Trace:
        src\Microsoft.VisualStudio.Threading.Tests.Shared\JoinableTaskTests.cs(3030,0): at Microsoft.VisualStudio.Threading.Tests.JoinableTaskTests.RunAsyncWithYieldingDelegateNestedInRunOverhead()

AsyncQueue.Completion has a fast path for the case where the result is cached. However, if the queue is already completed the first time this property is accessed, the backing field AsyncQueue.completedSource is never assigned. The fast path through this property could be improved by caching the TplExtensions.CompletedTask result so the lock doesn't need to be taken again.

This test has been observed to fail

   TransitionToMainThreadRaisedWhenSwitchingToMainThread [FAIL]
      Assert.Equal() Failure
      Expected: 1
      Actual:   0
      Stack Trace:
        src\Microsoft.VisualStudio.Threading.Tests.Shared\JoinableTaskTests.cs(529,0): at Microsoft.VisualStudio.Threading.Tests.JoinableTaskTests.<>c__DisplayClass19_0.<<TransitionToMainThreadRaisedWhenSwitchingToMainThread>b__0>d.MoveNext()
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        src\Microsoft.VisualStudio.Threading.Shared\JoinableTask.cs(519,0): at Microsoft.VisualStudio.Threading.JoinableTask.<JoinAsync>d__76.MoveNext()
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        src\Microsoft.VisualStudio.Threading.Shared\JoinableTask.cs(884,0): at Microsoft.VisualStudio.Threading.JoinableTask.CompleteOnCurrentThread()
        src\Microsoft.VisualStudio.Threading.Shared\JoinableTask.cs(497,0): at Microsoft.VisualStudio.Threading.JoinableTask.Join(CancellationToken cancellationToken)
        src\Microsoft.VisualStudio.Threading.Tests.Shared\JoinableTaskTests.cs(546,0): at Microsoft.VisualStudio.Threading.Tests.JoinableTaskTests.TransitionToMainThreadRaisedWhenSwitchingToMainThread()

Currently the order in which dequeuers are provided values from AsyncQueue<T> is unspecified. This can lead to strange behavior such as the following:

Single-threaded reordering (same cancellation token)

1. Thread A calls DequeueAsync with CancellationToken.None
2. Thread A calls DequeueAsync with CancellationToken.None
3. Thread A calls Enqueue(1)
4. Thread A calls Enqueue(2)

In this scenario, the queue will actually behave like a stack (LIFO) instead of a queue (FIFO) with providing values to the dequeuers.

Single-threaded reordering (different cancellation tokens)

1. Thread A calls DequeueAsync with CancellationToken.None
2. Thread A calls DequeueAsync with a different CancellationToken
3. Thread A calls Enqueue(1)
4. Thread A calls Enqueue(2)

In this scenario, the value provided to the dequeue operation is steps 1 and 2 could be in either order (unspecified).

Fairness problems

1. Thread A calls DequeueAsync with a cancellation token
2. Thread B calls DequeueAsync with CancellationToken.None, and the handler for this synchronously calls DequeueAsync with CancellationToken.None

In this scenario, if the ordering of the Dictionary provides orders CancellationToken.None before the other cancellation token, the dequeuer added by thread A will never receive a value, because the Enqueue operation will always prefer to provide a value for a dequeuer with CancellationToken.None before looking to any other value.

I believe this test would benefit from the following:

1. Verifying that the Task returned by dequeueTask.ContinueWith(...) completed successfully.
2. An assertion at the end that queue.Count==1.

The documentation as seen here is does not seem to match the actual test behavior. This could be due to a mistake in the comment, or possibly the comment is referring to cancellation tokens in some other part of the code?

Currently SignalAndWait resets remainingParticipants, suggesting that the AsyncBarrier can be used multiple times to let tasks proceed in groups of size participantCount. However, the reset of participantCount and waiters is not atomic, allowing for a race condition when the barrier is used in this manner. The semantics after the barrier lets the first group of tasks proceed could be implemented as any of the following mutually-exclusive options:

• The barrier is reset atomically, allowing reuse with groups of size participantCount
• After the group of tasks proceeds, subsequent calls to SignalAndWait throw an exception
• After the first group of tasks proceeds, subsequent calls to SignalAndWait return a completed future (i.e. proceed immediately)

I'm wondering why this use of ContinueWith in AsyncLazy<T> doesn't specify ExecuteSynchronously. It seems like that would be safe and marginally more efficient.

The AsyncReaderWriterLock class sometimes does a synchronous wait when disposing its lock, without blocking RPC re-entrancy. This can result in crashes or deadlocks when this is invoked from the UI thread and unwanted RPC calls get in on top.

See internal bug Bug 383244: [Dev15] VS crashes on Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DowngradeLockAsync

It's a given that an IVs*Events interface will (almost certainly) be invoked on the UI thread. So requiring explicitly checking is wasteful, as shown here:

// IVsSelectionEvents
public int OnCmdUIContextChanged(uint dwCmdUICookie, int fActive)
{
   ThreadHelper.ThrowIfNotOnUIThread(); // VSSDK002 should not demand this before other IVs* calls.
   ivssolution.DoSomething();
}

Similar argument for overrides of Package.Initialize or Package.Dispose.

When a JoinableTask completes at about the same time as another thread calls Post on its captured SynchronizationContext, a NullReferenceException can be thrown from this code when this.job is first checked to be not null, then on another thread the OnCompleted method sets it to null, then this Post method dereferences that field which was checked to not be null but now is.

The following method should trigger an analyzer on the call to SetProperty, but it does not. The analyzer sees the VerifyOnUIThread() "above" the call to SetProperty and considers that it must have executed.

using System;
using Microsoft.VisualStudio.Shell.Interop;

class Test {
    void F() {
        IVsSolution sln = null;
        if (false) {
            VerifyOnUIThread();
        }

        sln.SetProperty(1000, null);
    }

    void VerifyOnUIThread() {
    }
}

We should be able to fix this using Roslyn's SemanticModel.AnalyzeControlFlow method.

VSTHRD002 and VSTHRD102 misfire on this line:

const string ResultPropertyName = nameof(Task<int>.Result);

Obviously this is not an invocation of the property getter and thus shouldn't cause a diagnostic to be produced.

The AsyncManualResetEvent.SetAsync method is (or should be) obsolete, because it is now synchronously implemented. Can we get VSSDK008 to not recommend using the async version of the Set() method, perhaps by recognizing that the async alternative is deprecated?

A property like this:

internal Task Task { get; set; }

Produces a compiler warning:
VSSDK010: Use "Async" suffix in names of Task-returning methods.

VSTHRD010 doesn't fire for:

        public void TestMethodDoesntFire()
        {
            dynamic shell = ServiceProvider.GlobalProvider.GetService(typeof(Microsoft.VisualStudio.Shell.Interop.IVsShell));
            shell.something();
        }
        public object TestMethodDoesntFire2()
        {
            return (new object()) as Microsoft.VisualStudio.OLE.Interop.IServiceProvider;
        }

Though it correctly fires for:

        public void TestMethodDoesFire()
        {
            dynamic shell = ServiceProvider.GlobalProvider.GetService(typeof(IVsShell)) as MyCustomShell;
            shell.something();
        }
        public object TestMethodDoesFire2()
        {
            return (new object()) as Microsoft.VisualStudio.Shell.Interop.IVsShell;
        }

As an image:

The same issue happens for:
Microsoft.DiagnosticsHub.VisualStudio.SDK.IVsDiagnosticsHubService
and:
as Microsoft.DiagnosticsHub.IRecentListService

The following snippet will cause VSTHRD012 to misfire on the last line:

var jtctxt = new JoinableTaskContext();
var jtcollection = jtctxt.CreateCollection();
this.JoinableTaskFactory = new JoinableTaskFactory(jtcollection);

I believe this is because JoinableTaskCollection isn't recognized as providing a JoinableTaskFactory or JoinableTaskContext, yet an overload of the JTF ctor does take it.

As reported by @digeff

When Task.FromResult<object>(null) is found (or more generally Task.FromResult<T>(default(T))), and it assigned to a Task rather than Task<T> (or returned from a method with that return type), we should propose replacing with TplExtensions.CompletedTask or Task.CompletedTask because it reuses Task instances.