From d162c02687e98674bee960eed3baa69b54bb6d4d Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Mon, 12 Aug 2024 21:10:47 -0600 Subject: [PATCH] Add `JoinableTaskContext.CreateNoOpContext()` method The `JoinableTaskContext` constructor unfortunately has a poor design where folks passing in `null` for the `SynchronizationContext` may mistakenly believe that they're getting a no-op instance when they aren't, if `SynchronizationContext.Current != null`. In this change, I add a method that *definitely* does what they expect. I also add an analyzer and a code fix provider to help identify code that was likely written with the incorrect expectation to help them switch to the new method which matches their expectation, or suppress the warning with a more clear syntax. --- doc/analyzers/VSTHRD115.md | 38 +++++ doc/analyzers/index.md | 1 + .../FixUtils.cs | 19 +++ ...nableTaskContextCtorWithNullArgsCodeFix.cs | 131 ++++++++++++++++ .../Strings.Designer.cs | 36 +++++ .../Strings.resx | 12 ++ .../Types.cs | 6 + ...ableTaskContextCtorWithNullArgsAnalyzer.cs | 95 +++++++++++ .../JoinableTaskContext.cs | 59 ++++++- .../JoinableTaskFactory.cs | 6 +- .../net472/PublicAPI.Unshipped.txt | 3 +- .../net6.0-windows/PublicAPI.Unshipped.txt | 3 +- .../net6.0/PublicAPI.Unshipped.txt | 3 +- .../netstandard2.0/PublicAPI.Unshipped.txt | 3 +- ...JoinableTaskContextCtorWithNullArgTests.cs | 147 ++++++++++++++++++ .../JoinableTaskContextTests.cs | 53 +++++++ 16 files changed, 608 insertions(+), 7 deletions(-) create mode 100644 doc/analyzers/VSTHRD115.md create mode 100644 src/Microsoft.VisualStudio.Threading.Analyzers.CodeFixes/VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsCodeFix.cs create mode 100644 src/Microsoft.VisualStudio.Threading.Analyzers/VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsAnalyzer.cs create mode 100644 test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD115AvoidJoinableTaskContextCtorWithNullArgTests.cs diff --git a/doc/analyzers/VSTHRD115.md b/doc/analyzers/VSTHRD115.md new file mode 100644 index 000000000..5e1560911 --- /dev/null +++ b/doc/analyzers/VSTHRD115.md @@ -0,0 +1,38 @@ +# VSTHRD115 Avoid creating a JoinableTaskContext with an explicit `null` `SynchronizationContext` + +Constructing a `JoinableTaskContext` with an explicit `null` `SynchronizationContext` is not recommended as a means to construct an instance for use in unit tests or processes without a main thread. +This is because the constructor will automatically use `SynchronizationContext.Current` in lieu of a non-`null` argument. +If `SynchronizationContext.Current` happens to be non-`null`, the constructor may unexpectedly configure the new instance as if a main thread were present. + +## Examples of patterns that are flagged by this analyzer + +```csharp +void SetupJTC() { + this.jtc = new JoinableTaskContext(null, null); +} +``` + +This code *appears* to configure the `JoinableTaskContext` to not be associated with any `SynchronizationContext`. +But in fact it will be associated with the current `SynchronizationContext` if one is present. + +## Solution + +If you intended to inherit `SynchronizationContext.Current` to initialize with a main thread, +provide that value explicitly as the second argument to suppress the warning: + +```cs +void SetupJTC() { + this.jtc = new JoinableTaskContext(null, SynchronizationContext.Current); +} +``` + +If you intended to create a `JoinableTaskContext` for use in a unit test or in a process without a main thread, +call `JoinableTaskContext.CreateNoOpContext()` instead: + +```cs +void SetupJTC() { + this.jtc = JoinableTaskContext.CreateNoOpContext(); +} +``` + +Code fixes are offered to update code to either of the above patterns. diff --git a/doc/analyzers/index.md b/doc/analyzers/index.md index fcbbb5ab6..bf20d0337 100644 --- a/doc/analyzers/index.md +++ b/doc/analyzers/index.md @@ -28,6 +28,7 @@ ID | Title | Severity | Supports | Default diagnostic severity [VSTHRD112](VSTHRD112.md) | Implement `System.IAsyncDisposable` | Advisory | | Info [VSTHRD113](VSTHRD113.md) | Check for `System.IAsyncDisposable` | Advisory | | Info [VSTHRD114](VSTHRD114.md) | Avoid returning null from a `Task`-returning method. | Advisory | | Warning +[VSTHRD115](VSTHRD115.md) | Avoid creating a JoinableTaskContext with an explicit `null` `SynchronizationContext` | Advisory | | Warning [VSTHRD200](VSTHRD200.md) | Use `Async` naming convention | Guideline | [VSTHRD103](VSTHRD103.md) | Warning ## Severity descriptions diff --git a/src/Microsoft.VisualStudio.Threading.Analyzers.CodeFixes/FixUtils.cs b/src/Microsoft.VisualStudio.Threading.Analyzers.CodeFixes/FixUtils.cs index 8d9fac560..9639738b9 100644 --- a/src/Microsoft.VisualStudio.Threading.Analyzers.CodeFixes/FixUtils.cs +++ b/src/Microsoft.VisualStudio.Threading.Analyzers.CodeFixes/FixUtils.cs @@ -12,6 +12,9 @@ using Microsoft.CodeAnalysis.FindSymbols; using Microsoft.CodeAnalysis.Rename; using Microsoft.CodeAnalysis.Simplification; +using CSSyntax = Microsoft.CodeAnalysis.CSharp.Syntax; +using VB = Microsoft.CodeAnalysis.VisualBasic; +using VBSyntax = Microsoft.CodeAnalysis.VisualBasic.Syntax; namespace Microsoft.VisualStudio.Threading.Analyzers; @@ -330,6 +333,22 @@ internal static NameSyntax QualifyName(IReadOnlyList qualifiers, SimpleN internal static async Task GetSyntaxRootOrThrowAsync(this Document document, CancellationToken cancellationToken) => await document.GetSyntaxRootAsync(cancellationToken) ?? throw new InvalidOperationException("No syntax root could be obtained from the document."); + internal static (SyntaxNode? Creation, SyntaxNode[]? Arguments) FindObjectCreationSyntax(SyntaxNode startFrom) + { + if (startFrom is CSharpSyntaxNode && startFrom.FirstAncestorOrSelf() is { } csCreation) + { + return (csCreation, csCreation.ArgumentList?.Arguments.ToArray()); + } + else if (startFrom is VB.VisualBasicSyntaxNode && startFrom.FirstAncestorOrSelf() is { } vbCreation) + { + return (vbCreation, vbCreation.ArgumentList?.Arguments.ToArray()); + } + else + { + return (null, null); + } + } + private static CSharpSyntaxNode UpdateStatementsForAsyncMethod(CSharpSyntaxNode body, SemanticModel? semanticModel, bool hasResultValue, CancellationToken cancellationToken) { var blockBody = body as BlockSyntax; diff --git a/src/Microsoft.VisualStudio.Threading.Analyzers.CodeFixes/VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsCodeFix.cs b/src/Microsoft.VisualStudio.Threading.Analyzers.CodeFixes/VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsCodeFix.cs new file mode 100644 index 000000000..dbbd61137 --- /dev/null +++ b/src/Microsoft.VisualStudio.Threading.Analyzers.CodeFixes/VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsCodeFix.cs @@ -0,0 +1,131 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System.Collections.Immutable; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.Editing; + +namespace Microsoft.VisualStudio.Threading.Analyzers; + +[ExportCodeFixProvider(LanguageNames.CSharp)] +public class VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsCodeFix : CodeFixProvider +{ + public const string SuppressWarningEquivalenceKey = "SuppressWarning"; + + public const string UseFactoryMethodEquivalenceKey = "UseFactoryMethod"; + + private static readonly ImmutableArray ReusableFixableDiagnosticIds = ImmutableArray.Create( + VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsAnalyzer.Id); + + /// + public override ImmutableArray FixableDiagnosticIds => ReusableFixableDiagnosticIds; + + /// + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + foreach (Diagnostic diagnostic in context.Diagnostics) + { + SyntaxNode? root = await context.Document.GetSyntaxRootAsync(context.CancellationToken); + if (root is null) + { + continue; + } + + if (!diagnostic.Properties.TryGetValue(VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsAnalyzer.NodeTypePropertyName, out string? nodeType) || nodeType is null) + { + continue; + } + + context.RegisterCodeFix(CodeAction.Create(Strings.VSTHRD115_CodeFix_Suppress_Title, ct => this.SuppressDiagnostic(context, root, nodeType, diagnostic, ct), SuppressWarningEquivalenceKey), diagnostic); + + if (diagnostic.Properties.TryGetValue(VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsAnalyzer.UsesDefaultThreadPropertyName, out string? usesDefaultThreadString) && usesDefaultThreadString is "true") + { + context.RegisterCodeFix(CodeAction.Create(Strings.VSTHRD115_CodeFix_UseFactory_Title, ct => this.SwitchToFactory(context, root, nodeType, diagnostic, ct), UseFactoryMethodEquivalenceKey), diagnostic); + } + } + } + + private async Task SuppressDiagnostic(CodeFixContext context, SyntaxNode root, string nodeType, Diagnostic diagnostic, CancellationToken cancellationToken) + { + SyntaxGenerator generator = SyntaxGenerator.GetGenerator(context.Document); + SyntaxNode targetNode = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true); + + Compilation? compilation = await context.Document.Project.GetCompilationAsync(cancellationToken); + if (compilation is null) + { + return context.Document; + } + + ITypeSymbol? syncContext = compilation.GetTypeByMetadataName("System.Threading.SynchronizationContext"); + if (syncContext is null) + { + return context.Document; + } + + ITypeSymbol? jtc = compilation.GetTypeByMetadataName(Types.JoinableTaskContext.FullName); + if (jtc is null) + { + return context.Document; + } + + SyntaxNode syncContextCurrent = generator.MemberAccessExpression(generator.TypeExpression(syncContext, addImport: true), nameof(SynchronizationContext.Current)); + switch (nodeType) + { + case VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsAnalyzer.NodeTypeArgument: + root = root.ReplaceNode(targetNode, syncContextCurrent); + break; + case VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsAnalyzer.NodeTypeCreation: + (SyntaxNode? creationNode, SyntaxNode[]? args) = FixUtils.FindObjectCreationSyntax(targetNode); + if (creationNode is null || args is null) + { + return context.Document; + } + + SyntaxNode threadArg = args.Length >= 1 ? args[0] : generator.Argument(generator.NullLiteralExpression()); + SyntaxNode syncContextArg = generator.Argument(syncContextCurrent); + + root = root.ReplaceNode(creationNode, generator.ObjectCreationExpression(jtc, threadArg, syncContextArg)); + break; + } + + Document modifiedDocument = context.Document.WithSyntaxRoot(root); + return modifiedDocument; + } + + private async Task SwitchToFactory(CodeFixContext context, SyntaxNode root, string nodeType, Diagnostic diagnostic, CancellationToken cancellationToken) + { + SyntaxGenerator generator = SyntaxGenerator.GetGenerator(context.Document); + SyntaxNode targetNode = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true); + + Compilation? compilation = await context.Document.Project.GetCompilationAsync(cancellationToken); + if (compilation is null) + { + return context.Document; + } + + (SyntaxNode? creationExpression, _) = FixUtils.FindObjectCreationSyntax(targetNode); + if (creationExpression is null) + { + return context.Document; + } + + ITypeSymbol? jtc = compilation.GetTypeByMetadataName(Types.JoinableTaskContext.FullName); + if (jtc is null) + { + return context.Document; + } + + SyntaxNode factoryExpression = generator.InvocationExpression(generator.MemberAccessExpression(generator.TypeExpression(jtc, addImport: true), Types.JoinableTaskContext.CreateNoOpContext)); + + root = root.ReplaceNode(creationExpression, factoryExpression); + + Document modifiedDocument = context.Document.WithSyntaxRoot(root); + return modifiedDocument; + } +} diff --git a/src/Microsoft.VisualStudio.Threading.Analyzers/Strings.Designer.cs b/src/Microsoft.VisualStudio.Threading.Analyzers/Strings.Designer.cs index 0e0d5f400..b16b44432 100644 --- a/src/Microsoft.VisualStudio.Threading.Analyzers/Strings.Designer.cs +++ b/src/Microsoft.VisualStudio.Threading.Analyzers/Strings.Designer.cs @@ -583,6 +583,42 @@ internal static string VSTHRD114_Title { } } + /// + /// Looks up a localized string similar to Specify 'SynchronizationContext.Current' explicitly. + /// + internal static string VSTHRD115_CodeFix_Suppress_Title { + get { + return ResourceManager.GetString("VSTHRD115_CodeFix_Suppress_Title", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Use 'JoinableTaskContext.CreateNoOpContext' instead.. + /// + internal static string VSTHRD115_CodeFix_UseFactory_Title { + get { + return ResourceManager.GetString("VSTHRD115_CodeFix_UseFactory_Title", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Avoid creating JoinableTaskContext with 'null' as the value for the SynchronizationContext because behavior varies by the value of SynchronizationContext.Current. + /// + internal static string VSTHRD115_MessageFormat { + get { + return ResourceManager.GetString("VSTHRD115_MessageFormat", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Avoid creating JoinableTaskContext with null SynchronizationContext. + /// + internal static string VSTHRD115_Title { + get { + return ResourceManager.GetString("VSTHRD115_Title", resourceCulture); + } + } + /// /// Looks up a localized string similar to Use "Async" suffix in names of methods that return an awaitable type. /// diff --git a/src/Microsoft.VisualStudio.Threading.Analyzers/Strings.resx b/src/Microsoft.VisualStudio.Threading.Analyzers/Strings.resx index 40b76e27f..97e248e4d 100644 --- a/src/Microsoft.VisualStudio.Threading.Analyzers/Strings.resx +++ b/src/Microsoft.VisualStudio.Threading.Analyzers/Strings.resx @@ -341,4 +341,16 @@ Start the work within this context, or use JoinableTaskFactory.RunAsync to start Use 'Task.FromResult' instead "Task.FromResult" should not be translated. + + Avoid creating JoinableTaskContext with null SynchronizationContext + + + Avoid creating JoinableTaskContext with 'null' as the value for the SynchronizationContext because behavior varies by the value of SynchronizationContext.Current + + + Specify 'SynchronizationContext.Current' explicitly + + + Use 'JoinableTaskContext.CreateNoOpContext' instead. + \ No newline at end of file diff --git a/src/Microsoft.VisualStudio.Threading.Analyzers/Types.cs b/src/Microsoft.VisualStudio.Threading.Analyzers/Types.cs index 54ec2acdc..8762d3a01 100644 --- a/src/Microsoft.VisualStudio.Threading.Analyzers/Types.cs +++ b/src/Microsoft.VisualStudio.Threading.Analyzers/Types.cs @@ -11,6 +11,8 @@ namespace Microsoft.VisualStudio.Threading.Analyzers; /// internal static class Types { + private const string VSThreadingNamespace = "Microsoft.VisualStudio.Threading"; + internal static class BclAsyncDisposable { internal const string FullName = "System.IAsyncDisposable"; @@ -131,6 +133,10 @@ internal static class JoinableTaskCollection internal static class JoinableTaskContext { internal const string TypeName = "JoinableTaskContext"; + + internal const string FullName = $"{VSThreadingNamespace}.{TypeName}"; + + internal const string CreateNoOpContext = "CreateNoOpContext"; } internal static class JoinableTask diff --git a/src/Microsoft.VisualStudio.Threading.Analyzers/VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsAnalyzer.cs b/src/Microsoft.VisualStudio.Threading.Analyzers/VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsAnalyzer.cs new file mode 100644 index 000000000..92360240b --- /dev/null +++ b/src/Microsoft.VisualStudio.Threading.Analyzers/VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsAnalyzer.cs @@ -0,0 +1,95 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System; +using System.Collections.Immutable; +using System.Linq; +using System.Threading; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; + +namespace Microsoft.VisualStudio.Threading.Analyzers; + +/// +/// Flags the use of new JoinableTaskContext(null, null) and advises using JoinableTaskContext.CreateNoOpContext instead. +/// +[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)] +public class VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsAnalyzer : DiagnosticAnalyzer +{ + public const string Id = "VSTHRD115"; + + internal const string UsesDefaultThreadPropertyName = "UsesDefaultThread"; + + internal const string NodeTypePropertyName = "NodeType"; + + internal const string NodeTypeArgument = "Argument"; + + internal const string NodeTypeCreation = "Creation"; + + internal static readonly DiagnosticDescriptor Descriptor = new DiagnosticDescriptor( + id: Id, + title: new LocalizableResourceString(nameof(Strings.VSTHRD115_Title), Strings.ResourceManager, typeof(Strings)), + messageFormat: new LocalizableResourceString(nameof(Strings.VSTHRD115_MessageFormat), Strings.ResourceManager, typeof(Strings)), + description: null, + helpLinkUri: Utils.GetHelpLink(Id), + category: "Usage", + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true); + + /// + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Descriptor); + + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze); + + context.RegisterCompilationStartAction(startCompilation => + { + INamedTypeSymbol? joinableTaskContextType = startCompilation.Compilation.GetTypeByMetadataName(Types.JoinableTaskContext.FullName); + if (joinableTaskContextType is not null) + { + IMethodSymbol? problematicCtor = joinableTaskContextType.InstanceConstructors.SingleOrDefault(ctor => ctor.Parameters.Length == 2 && ctor.Parameters[0].Type.Name == nameof(Thread) && ctor.Parameters[1].Type.Name == nameof(SynchronizationContext)); + + if (problematicCtor is not null && joinableTaskContextType.GetMembers(Types.JoinableTaskContext.CreateNoOpContext).Length > 0) + { + startCompilation.RegisterOperationAction(Utils.DebuggableWrapper(c => this.AnalyzeObjectCreation(c, joinableTaskContextType, problematicCtor)), OperationKind.ObjectCreation); + } + } + }); + } + + private void AnalyzeObjectCreation(OperationAnalysisContext context, INamedTypeSymbol joinableTaskContextType, IMethodSymbol problematicCtor) + { + IObjectCreationOperation creation = (IObjectCreationOperation)context.Operation; + if (SymbolEqualityComparer.Default.Equals(creation.Constructor, problematicCtor)) + { + // Only flag if "null" is passed in as the constructor's second argument (explicitly or implicitly). + if (creation.Arguments.Length == 2) + { + IOperation arg2 = creation.Arguments[1].Value; + if (arg2 is IConversionOperation { Operand: ILiteralOperation { ConstantValue: { HasValue: true, Value: null } } literal }) + { + context.ReportDiagnostic(Diagnostic.Create(Descriptor, literal.Syntax.GetLocation(), CreateProperties(NodeTypeArgument))); + } + else if (arg2 is IDefaultValueOperation { ConstantValue: { HasValue: true, Value: null } }) + { + context.ReportDiagnostic(Diagnostic.Create(Descriptor, creation.Syntax.GetLocation(), CreateProperties(NodeTypeCreation))); + } + + ImmutableDictionary CreateProperties(string nodeType) + { + // The caller is using the default thread if they omit the argument, pass in "null", or pass in "Thread.CurrentThread". + // At the moment, we are not testing for the Thread.CurrentThread case. + bool usesDefaultThread = creation.Arguments[0].Value is IDefaultValueOperation { ConstantValue: { HasValue: true, Value: null } } + or IConversionOperation { Operand: ILiteralOperation { ConstantValue: { HasValue: true, Value: null } } }; + + return ImmutableDictionary.Create() + .Add(UsesDefaultThreadPropertyName, usesDefaultThread ? "true" : "false") + .Add(NodeTypePropertyName, nodeType); + } + } + } + } +} diff --git a/src/Microsoft.VisualStudio.Threading/JoinableTaskContext.cs b/src/Microsoft.VisualStudio.Threading/JoinableTaskContext.cs index 3eb58fbcb..332f8945f 100644 --- a/src/Microsoft.VisualStudio.Threading/JoinableTaskContext.cs +++ b/src/Microsoft.VisualStudio.Threading/JoinableTaskContext.cs @@ -153,6 +153,17 @@ public partial class JoinableTaskContext : IDisposable /// will provide the means to switch /// to the main thread from another thread. /// + /// + /// + /// When is at the time this constructor is invoked, + /// requests to switch to the main thread using + /// will not result in any thread switch. + /// This is appropriate for unit test environments where there is no main thread to switch to or processes + /// which otherwise do not define a main thread. + /// Thread safety concern: When configured without a synchronization context, code that requests the main thread + /// as a means of avoiding concurrency may malfunction due to data race conditions. + /// + /// public JoinableTaskContext() : this(Thread.CurrentThread, SynchronizationContext.Current) { @@ -163,10 +174,19 @@ public JoinableTaskContext() /// /// /// The thread to switch to in . - /// If omitted, the current thread will be assumed to be the main thread. + /// If , the current thread will be assumed to be the main thread. /// /// - /// The synchronization context to use to switch to the main thread. + /// The synchronization context to use to switch to the main thread. + /// + /// If is specified (or the argument is omitted), the current synchronization context will be used. + /// If is also , + /// requests to switch to the main thread using will not result in any thread switch. + /// This is appropriate for unit test environments where there is no main thread to switch to or processes + /// which otherwise do not define a main thread. + /// Thread safety concern: When configured without a synchronization context, code that requests the main thread + /// as a means of avoiding concurrency may malfunction due to data race conditions. + /// /// public JoinableTaskContext(Thread? mainThread = null, SynchronizationContext? synchronizationContext = null) { @@ -282,6 +302,41 @@ protected internal virtual SynchronizationContext NoMessagePumpSynchronizationCo } } + /// + /// Initializes a new instance of the class + /// that is configured to no-op on calls to . + /// + /// A new instance of . + /// + /// + /// This method is equivalent to calling the constructor + /// with the property first set to . + /// This entry point however will have the same behavior regardless of the value of . + /// + /// + /// The caller's thread will still be captured for use by such properties as + /// and . + /// These properties generally have no effect except as used by application-specific code beyond this library. + /// + /// + /// This method is useful for creating a in a unit test environment + /// or in a process that does not have a main thread, but which includes code that requires an instance + /// of or . + /// Such code can receive the instance returned by this method and use it in a normal way but no main thread switches will be honored. + /// + /// + /// Thread safety concern: Because main thread switches will not be honored, code that requests the main thread + /// as a means of avoiding concurrency may malfunction due to data race conditions. + /// + /// + public static JoinableTaskContext CreateNoOpContext() + { + using (((SynchronizationContext?)null).Apply()) + { + return new JoinableTaskContext(); + } + } + /// /// Conceals any JoinableTask the caller is associated with until the returned value is disposed. /// diff --git a/src/Microsoft.VisualStudio.Threading/JoinableTaskFactory.cs b/src/Microsoft.VisualStudio.Threading/JoinableTaskFactory.cs index a93f5fe87..018f4c4bc 100644 --- a/src/Microsoft.VisualStudio.Threading/JoinableTaskFactory.cs +++ b/src/Microsoft.VisualStudio.Threading/JoinableTaskFactory.cs @@ -130,7 +130,7 @@ protected SynchronizationContext? UnderlyingSynchronizationContext } /// - /// Gets an awaitable whose continuations execute on the synchronization context that this instance was initialized with, + /// Gets an awaitable whose continuations execute on the main thread, /// in such a way as to mitigate both deadlocks and reentrancy. /// /// @@ -159,6 +159,10 @@ protected SynchronizationContext? UnderlyingSynchronizationContext /// } /// /// + /// + /// When the owning is created with a , + /// this method has no effect and the caller will continue execution on its original thread. + /// /// public MainThreadAwaitable SwitchToMainThreadAsync(CancellationToken cancellationToken = default(CancellationToken)) { diff --git a/src/Microsoft.VisualStudio.Threading/net472/PublicAPI.Unshipped.txt b/src/Microsoft.VisualStudio.Threading/net472/PublicAPI.Unshipped.txt index bb9b5f944..0e00ec898 100644 --- a/src/Microsoft.VisualStudio.Threading/net472/PublicAPI.Unshipped.txt +++ b/src/Microsoft.VisualStudio.Threading/net472/PublicAPI.Unshipped.txt @@ -1 +1,2 @@ -Microsoft.VisualStudio.Threading.AsyncBarrier.SignalAndWait(System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.ValueTask \ No newline at end of file +Microsoft.VisualStudio.Threading.AsyncBarrier.SignalAndWait(System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.ValueTask +static Microsoft.VisualStudio.Threading.JoinableTaskContext.CreateNoOpContext() -> Microsoft.VisualStudio.Threading.JoinableTaskContext! \ No newline at end of file diff --git a/src/Microsoft.VisualStudio.Threading/net6.0-windows/PublicAPI.Unshipped.txt b/src/Microsoft.VisualStudio.Threading/net6.0-windows/PublicAPI.Unshipped.txt index bb9b5f944..0e00ec898 100644 --- a/src/Microsoft.VisualStudio.Threading/net6.0-windows/PublicAPI.Unshipped.txt +++ b/src/Microsoft.VisualStudio.Threading/net6.0-windows/PublicAPI.Unshipped.txt @@ -1 +1,2 @@ -Microsoft.VisualStudio.Threading.AsyncBarrier.SignalAndWait(System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.ValueTask \ No newline at end of file +Microsoft.VisualStudio.Threading.AsyncBarrier.SignalAndWait(System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.ValueTask +static Microsoft.VisualStudio.Threading.JoinableTaskContext.CreateNoOpContext() -> Microsoft.VisualStudio.Threading.JoinableTaskContext! \ No newline at end of file diff --git a/src/Microsoft.VisualStudio.Threading/net6.0/PublicAPI.Unshipped.txt b/src/Microsoft.VisualStudio.Threading/net6.0/PublicAPI.Unshipped.txt index bb9b5f944..0e00ec898 100644 --- a/src/Microsoft.VisualStudio.Threading/net6.0/PublicAPI.Unshipped.txt +++ b/src/Microsoft.VisualStudio.Threading/net6.0/PublicAPI.Unshipped.txt @@ -1 +1,2 @@ -Microsoft.VisualStudio.Threading.AsyncBarrier.SignalAndWait(System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.ValueTask \ No newline at end of file +Microsoft.VisualStudio.Threading.AsyncBarrier.SignalAndWait(System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.ValueTask +static Microsoft.VisualStudio.Threading.JoinableTaskContext.CreateNoOpContext() -> Microsoft.VisualStudio.Threading.JoinableTaskContext! \ No newline at end of file diff --git a/src/Microsoft.VisualStudio.Threading/netstandard2.0/PublicAPI.Unshipped.txt b/src/Microsoft.VisualStudio.Threading/netstandard2.0/PublicAPI.Unshipped.txt index bb9b5f944..0e00ec898 100644 --- a/src/Microsoft.VisualStudio.Threading/netstandard2.0/PublicAPI.Unshipped.txt +++ b/src/Microsoft.VisualStudio.Threading/netstandard2.0/PublicAPI.Unshipped.txt @@ -1 +1,2 @@ -Microsoft.VisualStudio.Threading.AsyncBarrier.SignalAndWait(System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.ValueTask \ No newline at end of file +Microsoft.VisualStudio.Threading.AsyncBarrier.SignalAndWait(System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.ValueTask +static Microsoft.VisualStudio.Threading.JoinableTaskContext.CreateNoOpContext() -> Microsoft.VisualStudio.Threading.JoinableTaskContext! \ No newline at end of file diff --git a/test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD115AvoidJoinableTaskContextCtorWithNullArgTests.cs b/test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD115AvoidJoinableTaskContextCtorWithNullArgTests.cs new file mode 100644 index 000000000..8603eb8c9 --- /dev/null +++ b/test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD115AvoidJoinableTaskContextCtorWithNullArgTests.cs @@ -0,0 +1,147 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using CSVerify = Microsoft.VisualStudio.Threading.Analyzers.Tests.CSharpCodeFixVerifier; +using VBVerify = Microsoft.VisualStudio.Threading.Analyzers.Tests.VisualBasicCodeFixVerifier; + +public class VSTHRD115AvoidJoinableTaskContextCtorWithNullArgTests +{ + private const string CSPreamble = """ + using System.Threading; + using Microsoft.VisualStudio.Threading; + + """; + + private const string VBPreamble = """ + Imports System.Threading + Imports Microsoft.VisualStudio.Threading + + """; + + [Fact] + public async Task ConstructorWithImplicitNullSyncContext_SuppressWarning_CS() + { + var test = CSPreamble + """ + class Test + { + void Create1() => [|new JoinableTaskContext(null)|]; + void Create2() => [|new JoinableTaskContext(Thread.CurrentThread)|]; + } + """; + + var withFix = CSPreamble + """ + class Test + { + void Create1() => new JoinableTaskContext(null, SynchronizationContext.Current); + void Create2() => new JoinableTaskContext(Thread.CurrentThread, SynchronizationContext.Current); + } + """; + + await new CSVerify.Test + { + TestCode = test, + FixedCode = withFix, + CodeActionEquivalenceKey = VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsCodeFix.SuppressWarningEquivalenceKey, + }.RunAsync(); + } + + [Fact] + public async Task ConstructorWithExplicitNullSyncContext_SuppressWarning() + { + var test = CSPreamble + """ + class Test + { + void Create1() => new JoinableTaskContext(null, [|null|]); + void Create2() => new JoinableTaskContext(Thread.CurrentThread, [|null|]); + } + """; + + var withFix = CSPreamble + """ + class Test + { + void Create1() => new JoinableTaskContext(null, SynchronizationContext.Current); + void Create2() => new JoinableTaskContext(Thread.CurrentThread, SynchronizationContext.Current); + } + """; + + await new CSVerify.Test + { + TestCode = test, + FixedCode = withFix, + CodeActionEquivalenceKey = VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsCodeFix.SuppressWarningEquivalenceKey, + }.RunAsync(); + } + + [Fact] + public async Task ConstructorWithNonDefaultThread_SuppressAction() + { + var test = CSPreamble + """ + class Test + { + void Create2(Thread thread) => [|new JoinableTaskContext(thread)|]; + void Create1(Thread thread) => new JoinableTaskContext(thread, [|null|]); + } + """; + + var withFix = CSPreamble + """ + class Test + { + void Create2(Thread thread) => new JoinableTaskContext(thread, SynchronizationContext.Current); + void Create1(Thread thread) => new JoinableTaskContext(thread, SynchronizationContext.Current); + } + """; + + await new CSVerify.Test + { + TestCode = test, + FixedCode = withFix, + CodeActionEquivalenceKey = VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsCodeFix.SuppressWarningEquivalenceKey, + }.RunAsync(); + } + + [Fact] + public async Task ConstructorWithNonDefaultThread_GetsNoFactoryAction() + { + var test = CSPreamble + """ + class Test + { + void Create2(Thread thread) => [|new JoinableTaskContext(thread)|]; + void Create1(Thread thread) => new JoinableTaskContext(thread, [|null|]); + } + """; + + await new CSVerify.Test + { + TestCode = test, + FixedCode = test, // no fix offered + CodeActionEquivalenceKey = VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsCodeFix.UseFactoryMethodEquivalenceKey, + }.RunAsync(); + } + + [Fact] + public async Task ConstructorWithNullSyncContext_UseFactoryMethod() + { + var test = CSPreamble + """ + class Test + { + void Create1() => [|new JoinableTaskContext(null)|]; + void Create2() => new JoinableTaskContext(null, [|null|]); + } + """; + + var withFix = CSPreamble + """ + class Test + { + void Create1() => JoinableTaskContext.CreateNoOpContext(); + void Create2() => JoinableTaskContext.CreateNoOpContext(); + } + """; + + await new CSVerify.Test + { + TestCode = test, + FixedCode = withFix, + CodeActionEquivalenceKey = VSTHRD115AvoidJoinableTaskContextCtorWithNullArgsCodeFix.UseFactoryMethodEquivalenceKey, + }.RunAsync(); + } +} diff --git a/test/Microsoft.VisualStudio.Threading.Tests/JoinableTaskContextTests.cs b/test/Microsoft.VisualStudio.Threading.Tests/JoinableTaskContextTests.cs index e50d64078..93dfffa59 100644 --- a/test/Microsoft.VisualStudio.Threading.Tests/JoinableTaskContextTests.cs +++ b/test/Microsoft.VisualStudio.Threading.Tests/JoinableTaskContextTests.cs @@ -7,6 +7,7 @@ using System.Threading; using System.Threading.Tasks; using System.Xml.Linq; +using Microsoft; using Microsoft.VisualStudio.Threading; using Xunit; using Xunit.Abstractions; @@ -718,6 +719,58 @@ public void Disposable() disposable.Dispose(); } + [Fact] + public void Ctor_ExplicitNullSyncContext() + { + this.SimulateUIThread(async delegate + { + Thread mainThread = Thread.CurrentThread; + Assumes.NotNull(SynchronizationContext.Current); + JoinableTaskContext jtc = JoinableTaskContext.CreateNoOpContext(); + await TaskScheduler.Default.SwitchTo(alwaysYield: true); // Get off the main thread. + Assert.NotSame(mainThread, Thread.CurrentThread); + + // Verify that switching to the main thread is a no-op. + Thread threadpoolThread = Thread.CurrentThread; + await jtc.Factory.SwitchToMainThreadAsync(this.TimeoutToken); + Assert.Same(threadpoolThread, Thread.CurrentThread); + }); + } + + [Fact] + public void Ctor_NullSyncContextArg_AmbientSyncContext() + { + this.SimulateUIThread(async delegate + { + Thread mainThread = Thread.CurrentThread; + Assumes.NotNull(SynchronizationContext.Current); + JoinableTaskContext jtc = new(null, null); + await TaskScheduler.Default.SwitchTo(alwaysYield: true); // Get off the main thread. + Assert.NotSame(mainThread, Thread.CurrentThread); + + // Verify that switching to the main thread works. + await jtc.Factory.SwitchToMainThreadAsync(this.TimeoutToken); + Assert.Same(mainThread, Thread.CurrentThread); + }); + } + + [Fact] + public void Ctor_Default() + { + this.SimulateUIThread(async delegate + { + Thread mainThread = Thread.CurrentThread; + Assumes.NotNull(SynchronizationContext.Current); + JoinableTaskContext jtc = new(); + await TaskScheduler.Default.SwitchTo(alwaysYield: true); // Get off the main thread. + Assert.NotSame(mainThread, Thread.CurrentThread); + + // Verify that switching to the main thread works. + await jtc.Factory.SwitchToMainThreadAsync(this.TimeoutToken); + Assert.Same(mainThread, Thread.CurrentThread); + }); + } + protected override JoinableTaskContext CreateJoinableTaskContext() { return new JoinableTaskContextDerived();