Bugfix/atr 975 fix rename hanging 2

docs/dev/CodingGuidelines/CodingGuidelines.md

@@ -355,8 +355,10 @@ You should avoid the use of `Task.Result` and `Task.Wait()` because this can cau
 
 For details, see the following articles: 
 
+* [Most recent practices recorded in the AI agent skill](https://github.com/madskristensen/vs-agent-plugins/blob/master/skills/handling-async-threading/SKILL.md)
 * [How to: Manage multiple threads in managed code](https://docs.microsoft.com/en-us/visualstudio/extensibility/managing-multiple-threads-in-managed-code) 
 * [Asynchronous and multithreaded programming within VS using the JoinableTaskFactory](https://blogs.msdn.microsoft.com/andrewarnottms/2014/05/07/asynchronous-and-multithreaded-programming-within-vs-using-the-joinabletaskfactory/)
+  - [Another link to the same article](https://docs.microsoft.com/en-us/archive/blogs/andrewarnott/asynchronous-and-multithreaded-programming-within-vs-using-the-joinabletaskfactory)
 * [Cookbook for Visual Studio](https://github.com/Microsoft/vs-threading/blob/master/doc/cookbook_vs.md)
 * [Three Threading Rules](https://github.com/Microsoft/vs-threading/blob/master/doc/threading_rules.md)
 

src/Acuminator/Acuminator.Analyzers/Acuminator.Analyzers.csproj

@@ -3,7 +3,7 @@
     <Title>Acuminator Analyzers</Title>
     <AssemblyTitle>Acuminator.Analyzers</AssemblyTitle>
     <TargetFramework>netstandard2.0</TargetFramework>
-    <Version>4.0.1</Version>
+    <Version>4.1.0</Version>
     <LangVersion>13.0</LangVersion>
     <SignAssembly>False</SignAssembly>
     <Company>Acumatica, Inc.</Company>

src/Acuminator/Acuminator.Runner.NetFramework/Acuminator.Runner.NetFramework.csproj

@@ -7,7 +7,7 @@
 		<TargetFramework>net48</TargetFramework>
 		<Deterministic>True</Deterministic>
 		<LangVersion>13.0</LangVersion>
-		<Version>4.0.1</Version>
+		<Version>4.1.0</Version>
 		<Nullable>enable</Nullable>
 		<WarningLevel>9999</WarningLevel>
 		<NeutralLanguage>en</NeutralLanguage>

src/Acuminator/Acuminator.Tests/Acuminator.Tests.csproj

@@ -5,7 +5,7 @@
     <AssemblyTitle>Acuminator.Tests</AssemblyTitle>
     <!-- Target framework must be net48 because net481 is not supported by our build system -->
     <TargetFramework>net48</TargetFramework>
-    <Version>4.0.1</Version>
+    <Version>4.1.0</Version>
     <LangVersion>13.0</LangVersion>
     <Nullable>enable</Nullable>
     <WarningLevel>9999</WarningLevel>
@@ -124,7 +124,7 @@
     <PackageReference Include="System.Collections.Immutable" Version="8.0.0" />
     <PackageReference Include="System.Memory" Version="4.6.3" />
     <PackageReference Include="System.Reflection.Primitives" Version="4.3.0" />
-    <PackageReference Include="System.Runtime.Handles" Version="4.0.1" />
+    <PackageReference Include="System.Runtime.Handles" Version="4.1.0" />
     <PackageReference Include="xunit" Version="2.4.0" />
     <PackageReference Include="xunit.abstractions" Version="2.0.3" />
     <PackageReference Include="xunit.analyzers" Version="0.10.0" />

src/Acuminator/Acuminator.Utilities/Acuminator.Utilities.csproj

@@ -3,7 +3,7 @@
     <Title>Acuminator Utilities</Title>
     <AssemblyTitle>Acuminator.Utilities</AssemblyTitle>
     <TargetFramework>netstandard2.0</TargetFramework>
-    <Version>4.0.1</Version>
+    <Version>4.1.0</Version>
     <NeutralLanguage>en</NeutralLanguage>
     <Description>Acuminator.Utilities library with shared analysis helpers</Description>
     <Company>Acumatica, Inc.</Company>

src/Acuminator/Acuminator.Vsix/AcuminatorVSPackage.cs

@@ -75,7 +75,7 @@ public sealed class AcuminatorVSPackage : AsyncPackage
 		private const string SettingsCategoryName = SharedConstants.PackageName;
 
 		public const string PackageName = SharedConstants.PackageName;
-		public const string PackageVersion = "4.0.1";
+		public const string PackageVersion = "4.1.0";
 
 		/// <summary>
 		/// AcuminatorVSPackage GUID string.
@@ -91,12 +91,38 @@ public sealed class AcuminatorVSPackage : AsyncPackage
 
 		private const int INSTANCE_UNINITIALIZED = 0;
 		private const int INSTANCE_INITIALIZED = 1;
-		private static int _instanceInitialized;
+		private static int _instanceInitialized = INSTANCE_UNINITIALIZED;
+
+		private const int NOT_DISPOSED = 0;
+		private const int DISPOSED = 1;
+		private volatile int _isDisposed = NOT_DISPOSED;
 
 		private OutOfProcessSettingsUpdater? _outOfProcessSettingsUpdater;
 
 		public static AcuminatorVSPackage Instance { get; private set; } = null!;
 
+
+		/// <summary>
+		/// The <see cref="JoinableTaskFactory"/> instance initialized for the <see cref="AsyncPackage"/>.<br/>
+		/// If the package is not yet initialized or already disposed, <see cref="Microsoft.VisualStudio.Shell.ThreadHelper.JoinableTaskFactory"/> is returned instead.
+		/// </summary>
+		/// <remarks>
+		/// According to VS cookbook and VS team's discussion, the <see cref="AsyncPackage.JoinableTaskFactory"/> should be preferred over <see cref="Microsoft.VisualStudio.Shell.ThreadHelper.JoinableTaskFactory"/>:
+		/// <list type="bullet">
+		/// <item>https://github.com/VsixCommunity/Community.VisualStudio.Toolkit/issues/24</item>
+		/// <item>https://microsoft.github.io/VSSDK-Analyzers/analyzers/VSSDK007.html</item>
+		/// </list>
+		/// Both factories are created from the same <see cref="JoinableTaskContext"/> — the one bound to the VS main thread.<br/>
+		/// So, they have identical participation in the JTF dependency graph that prevents deadlocks on the UI thread. Swapping one for the other changes nothing about deadlock behavior.<br/>
+		/// The difference is the <see cref="JoinableTaskCollection"/>. <see cref="AsyncPackage.JoinableTaskFactory"/> has its own collection, and package disposal drains it.<br/> 
+		/// The work you started can't still be running against torn-down state after the package unloads.<br/>
+		/// On the other hand, <see cref="Microsoft.VisualStudio.Shell.ThreadHelper.JoinableTaskFactory"/> is ambient and tracks nothing on your behalf. That's the reason behind VSSDK007 diagnostic.
+		/// </remarks>
+		public static JoinableTaskFactory JTF =>
+			Instance?._isDisposed == NOT_DISPOSED
+				? Instance.JoinableTaskFactory
+				: ThreadHelper.JoinableTaskFactory;
+
 		private readonly Lazy<GeneralOptionsPage?> _generalOptionsPage =
 			new(() => Instance.GetDialogPage(typeof(GeneralOptionsPage)) as GeneralOptionsPage, isThreadSafe: true);
 
@@ -345,6 +371,21 @@ private async System.Threading.Tasks.Task<bool> IsSolutionLoadedAsync()
 		protected override void Dispose(bool disposing)
 		{
 			base.Dispose(disposing);
+
+			// It is important to set flag after the base call to Dispose to avoid rare but possible VS hanging on package unload. 
+			// The _isDisposed flag check on JTF property prevents returning JTF from a disposed package. But if the code flips it before base.Dispose call, there will be a problem. 
+			// The base AsyncPackage.Dispose(bool) method does: disposeCancellationTokenSource.Cancel() -> ThreadHelper.JoinableTaskFactory.Run(JoinableTaskCollection.JoinTillEmptyAsync) — no token, no timeout —> Package.Dispose. 
+			// So the main thread blocks until every JoinableTask in the package collection finishes.
+			// During that drain, AcuminatorVSPackage.JTF already returns ThreadHelper's factory. A FileAndForgetAcuminatorTask wrapper started before shutdown is a member of the package collection (so the drain waits on it) 
+			// and awaits a foreign task. When that foreign task resumes and needs its main-thread hop via AcuminatorVSPackage.JTF.SwitchToMainThreadAsync(), RequestSwitchToMainThread (with a null ambient job) creates a transient 
+			// on ThreadHelper's factory — which has no collection, so the transient is not in the drained graph.A main thread blocked in Run pumps only joined work, 
+			// so that continuation never runs -> the foreign task never completes -> the wrapper never completes -> JoinTillEmptyAsync never returns -> indefinite hang on close.
+			//
+			// Had the flag been set after base.Dispose, the same hop would go through the package factory, land in the collection, and be pumped by the drain — no hang. 
+			// The "removed redundant cancellation tokens" commit compounds it: those switches no longer observe DisposalToken, so in-flight work can't self-cancel to escape the wait either.
+			if (Interlocked.Exchange(ref _isDisposed, DISPOSED) == DISPOSED)
+				return;
+
 			AcuminatorLogger?.Dispose();
 			_outOfProcessSettingsUpdater?.Dispose();
 

src/Acuminator/Acuminator.Vsix/Coloriser/AsyncTagging/BackgroundTagging.cs

@@ -51,7 +51,7 @@ public static BackgroundTagging StartBackgroundTagging(PXRoslynColorizerTagger t
 																	 _vsTaskScheduler);
 
 			// ContinueWith schedules the lambda on the VS UI thread scheduler. The lambda runs on the UI thread and calls AfterTaggingActionAsync(...).
-			// Inside AfterTaggingActionAsync, the important path calls ThreadHelper.JoinableTaskFactory.RunAsync(tagger.RaiseTagsChangedAsync).Task 
+			// Inside AfterTaggingActionAsync, the important path calls AcuminatorVSPackage.JTF.RunAsync(tagger.RaiseTagsChangedAsync).Task 
 			// this starts RaiseTagsChangedAsync and immediately returns the underlying Task representing it (still running).
 			// The lambda returns that inner Task immediately — it does not await it.
 			// The outer Task<Task> stored in TaggingTask is marked as Completed (RanToCompletion) at this point, because the lambda has returned. 
@@ -124,7 +124,7 @@ private static Task AfterTaggingActionAsync(Task taggingTask, PXRoslynColorizerT
 			}
 
 			// We should be on UI thread here but the tagger.RaiseTagsChangedAsync switches to UI thread from non UI threads internally if needed
-			return Shell.ThreadHelper.JoinableTaskFactory.RunAsync(tagger.RaiseTagsChangedAsync).Task;
+			return AcuminatorVSPackage.JTF.RunAsync(tagger.RaiseTagsChangedAsync).Task;
 		}
 	}
 }

src/Acuminator/Acuminator.Vsix/Coloriser/Base/PXTaggerBase.cs

@@ -3,12 +3,16 @@
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
+using System.Runtime.CompilerServices;
+using System.Threading;
 using System.Threading.Tasks;
 
 using Acuminator.Utilities.Common;
 using Acuminator.Vsix.Settings;
+using Acuminator.Vsix.Utilities;
 
 using Microsoft.VisualStudio.Text;
+using Microsoft.VisualStudio.Text.Tagging;
 
 using ThreadHelper = Microsoft.VisualStudio.Shell.ThreadHelper;
 
@@ -55,6 +59,16 @@ protected PXTaggerBase(ITextBuffer buffer, ITextDocumentFactoryService textDocum
 			_disposedNotification.CurrentTextDocumentDisposed += CleanupOnTextDocumentDisposed;
 		}
 
+		protected static IEnumerable<ITagSpan<TTag>> GetIntersectionWithRequestedTags<TTag>(
+																				IReadOnlyCollection<ITagSpan<TTag>> tags,
+																				NormalizedSnapshotSpanCollection requestedSpans)
+		where TTag : ITag
+		{
+			return tags?.Count > 0
+				? tags.Where(tag => requestedSpans.IntersectsWith(tag.Span))
+				: [];
+		}
+
 		protected virtual void ColoringSettingChangedHandler(object sender, SettingChangedEventArgs e)
 		{
 			ColoringSettingsChanged = true;
@@ -64,11 +78,34 @@ protected virtual void ColoringSettingChangedHandler(object sender, SettingChang
 			RaiseTagsChanged();
 		}
 
+		/// <summary>
+		/// Raises the tags changed asynchronously and do not observe the raised task.
+		/// </summary>
+		/// <remarks>
+		/// The method is intended to be called from void-returning event handlers.
+		/// </remarks>
+		/// <param name="calledFrom">(Optional) The method raising the tag changed event.</param>
+		protected void RaiseTagsChangedAsyncAndForget([CallerMemberName] string? calledFrom = null)
+		{
+			if (ThreadHelper.CheckAccess())
+				RaiseTagsChanged();
+			else
+			{
+				string taggerName = this.GetType().Name;
+				calledFrom = calledFrom.NullIfWhiteSpace() ?? nameof(RaiseTagsChangedAsyncAndForget);
+
+				// See the VS cookbook for file and forget methods
+				// https://github.com/microsoft/vs-threading/blob/main/docfx/docs/cookbook_vs.md#task-returning-fire-and-forget-methods
+				var raiseTaggerChanged = () => RaiseTagsChangedAsync();
+				raiseTaggerChanged.FileAndForgetAcuminatorTask($"vs/{AcuminatorVSPackage.PackageName}/{taggerName}/{calledFrom}");
+			}
+		}
+
 		internal async Task RaiseTagsChangedAsync()
 		{
 			if (!ThreadHelper.CheckAccess())
 			{
-				await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync();
+				await AcuminatorVSPackage.JTF.SwitchToMainThreadAsync();
 			}
 
 			RaiseTagsChangedImpl();

src/Acuminator/Acuminator.Vsix/Coloriser/Outlining/PXOutliningTagger.cs

@@ -31,9 +31,9 @@ public PXOutliningTagger(ITextBuffer buffer, ITextDocumentFactoryService textDoc
 		{
 		}
 
-		public IEnumerable<ITagSpan<IOutliningRegionTag>> GetTags(NormalizedSnapshotSpanCollection spans)
+		public IEnumerable<ITagSpan<IOutliningRegionTag>> GetTags(NormalizedSnapshotSpanCollection requestedSpans)
 		{
-			if (spans == null || spans.Count == 0 || AcuminatorVSPackage.Instance?.UseBqlOutlining != true)
+			if (requestedSpans?.Count is null or 0 || AcuminatorVSPackage.Instance?.UseBqlOutlining != true)
 				return [];
 
 			if (ColorizerTagger == null)
@@ -48,7 +48,8 @@ public IEnumerable<ITagSpan<IOutliningRegionTag>> GetTags(NormalizedSnapshotSpan
 			if (!HasReferenceToAcumaticaPlatform)
 				return [];
 
-			return ColorizerTagger.OutliningsTagsCache.ProcessedTags;
+			var processedTags = ColorizerTagger.OutliningsTagsCache.ProcessedTags;
+			return GetIntersectionWithRequestedTags(processedTags, requestedSpans);
 		}
 
 		private static bool TryGetColorizingTaggerFromBuffer(ITextBuffer textBuffer, out PXRoslynColorizerTagger colorizingTagger)

src/Acuminator/Acuminator.Vsix/Coloriser/Outlining/PXOutliningTaggerProvider.cs

@@ -28,7 +28,7 @@ public PXOutliningTaggerProvider(ITextDocumentFactoryService textDocumentFactory
 
 	public ITagger<T>? CreateTagger<T>(ITextBuffer buffer) where T : ITag
 	{
-		if (buffer == null || !ThreadHelper.CheckAccess())
+		if (buffer == null || !typeof(ITagger<T>).IsAssignableFrom(typeof(PXOutliningTagger)) || !ThreadHelper.CheckAccess())
 			return null;
 
 		PXOutliningTagger outliningTagger = buffer.Properties.GetOrCreateSingletonProperty(typeof(PXOutliningTagger), () =>

src/Acuminator/Acuminator.Vsix/Coloriser/PXColorizerTaggerProvider.cs

@@ -53,8 +53,11 @@ public PXColorizerTaggerProvider(IClassificationTypeRegistryService classificati
 	public virtual ITagger<T>? CreateTagger<T>(ITextView textView, ITextBuffer textBuffer)
 	where T : ITag
 	{
-		if (textView == null || textBuffer == null || textView.TextBuffer != textBuffer || !ThreadHelper.CheckAccess())
+		if (textView == null || textBuffer == null || textView.TextBuffer != textBuffer || 
+			!typeof(ITagger<T>).IsAssignableFrom(typeof(PXRoslynColorizerTagger)) || !ThreadHelper.CheckAccess())
+		{
 			return null;
+		}
 
 		var tagger = textBuffer.Properties.GetOrCreateSingletonProperty(typeof(PXRoslynColorizerTagger), () =>
 		{

src/Acuminator/Acuminator.Vsix/Coloriser/PXRoslynColorizerTagger.cs

@@ -136,25 +136,28 @@ protected internal override void ResetCacheAndFlags(ITextSnapshot? newSnapshotTo
 	/// <summary>
 	/// Gets the tags asynchronously from the specified snapshot with Roslyn.
 	/// </summary>
-	/// <param name="spans">The spans for tagging. The current implementation doesn't take them into account and re-tags the entire document.</param>
+	/// <param name="requestedSpans">
+	/// The spans for tagging. The current implementation re-tags the entire document but returns the intersection with the requested spans.
+	/// </param>
 	/// <returns>
 	/// The current snapshot of the collected tags.
 	/// </returns>
-	public IEnumerable<ITagSpan<IClassificationTag>> GetTags(NormalizedSnapshotSpanCollection spans)
+	public IEnumerable<ITagSpan<IClassificationTag>> GetTags(NormalizedSnapshotSpanCollection requestedSpans)
 	{
-		if (spans?.Count is null or 0 || AcuminatorVSPackage.Instance?.ColoringEnabled != true || !HasReferenceToAcumaticaPlatform)
+		if (requestedSpans?.Count is null or 0 || AcuminatorVSPackage.Instance?.ColoringEnabled != true || !HasReferenceToAcumaticaPlatform)
 			return [];
 
 		var workspace = _roslynWorkspaceProvider.Workspace; 
 
 		if (workspace == null)
 			return [];
 
-		ITextSnapshot newSnapshotToTag = spans[0].Snapshot;
+		ITextSnapshot newSnapshotToTag = requestedSpans[0].Snapshot;
 
 		if (CheckIfParsingAndRetaggingIsNotNecessary(newSnapshotToTag))
 		{
-			return ClassificationTagsCache.ProcessedTags;
+			var cachedProcessedTags = ClassificationTagsCache.ProcessedTags;
+			return GetIntersectionWithRequestedTags(cachedProcessedTags, requestedSpans);
 		}
 
 		if (BackgroundTagging != null)
@@ -166,7 +169,8 @@ public IEnumerable<ITagSpan<IClassificationTag>> GetTags(NormalizedSnapshotSpanC
 		ResetCacheAndFlags(newSnapshotToTag);
 		BackgroundTagging = BackgroundTagging.StartBackgroundTagging(this);
 
-		return ClassificationTagsCache.ProcessedTags;
+		var processedTags = ClassificationTagsCache.ProcessedTags;
+		return GetIntersectionWithRequestedTags(processedTags, requestedSpans);
 	}
 
 	protected virtual bool CheckIfParsingAndRetaggingIsNotNecessary(ITextSnapshot newSnapshotToTag) =>
@@ -278,11 +282,7 @@ private void WorkspaceAttachedToDocumentChanged(object sender, DocumentWorkspace
 
 		// We need to raise the tags changed event to trigger re-coloring on workspace change
 		ResetCacheAndFlags(newSnapshotToCache: null);
-
-		if (ThreadHelper.CheckAccess())
-			RaiseTagsChanged();
-		else
-			ThreadHelper.JoinableTaskFactory.Run(RaiseTagsChangedAsync);
+		RaiseTagsChangedAsyncAndForget();
 	}
 
 	private void OnWorkspaceChanged(object sender, WorkspaceChangeEventArgs e)
@@ -329,12 +329,8 @@ private void OnWorkspaceChanged(object sender, WorkspaceChangeEventArgs e)
 		if (oldHasReferenceToAcumaticaPlatform != _hasReferenceToAcumaticaPlatform)
 		{
 			ResetCacheAndFlags(newSnapshotToCache: null);
-
-			if (ThreadHelper.CheckAccess())
-				RaiseTagsChanged();
-			else
-				ThreadHelper.JoinableTaskFactory.Run(RaiseTagsChangedAsync);
-		}	
+			RaiseTagsChangedAsyncAndForget();
+		}
 	}
 
 	private bool GetAcumaticaReferenceOnProjectChange(WorkspaceChangeEventArgs e, bool oldHasReferenceToAcumaticaPlatform)

src/Acuminator/Acuminator.Vsix/Coloriser/Roslyn/PXRoslynColorizerTagger.PXColorizerSyntaxWalker.cs

@@ -466,7 +466,7 @@ private void UpdateCodeEditorIfNecessary()
 				var cancellationToken = _cancellationToken;
 
 				#pragma warning disable VSTHRD110 // Observe result of async calls
-				Shell.ThreadHelper.JoinableTaskFactory.RunAsync(async () =>
+				AcuminatorVSPackage.JTF.RunAsync(async () =>
 				{
 					if (!cancellationToken.IsCancellationRequested)
 					{

src/Acuminator/Acuminator.Vsix/Commands/BQL Fixer/FixBqlCommand.cs

@@ -59,13 +59,15 @@ public static void Initialize(AsyncPackage package, OleMenuCommandService comman
 			}
 		}
 
-		protected override void CommandCallback(object sender, EventArgs e) =>
-			CommandCallbackAsync()
-				.FileAndForget($"vs/{AcuminatorVSPackage.PackageName}/{nameof(FixBqlCommand)}");
+		protected override void CommandCallback(object sender, EventArgs e)
+		{
+			var commandExecutor = () => CommandCallbackAsync();
+			commandExecutor.FileAndForgetAcuminatorTask($"vs/{AcuminatorVSPackage.PackageName}/{nameof(FixBqlCommand)}");
+		}
 
 		private async System.Threading.Tasks.Task CommandCallbackAsync()
 		{
-			await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync();
+			await AcuminatorVSPackage.JTF.SwitchToMainThreadAsync();
 			IWpfTextView? textView = await ServiceProvider.GetWpfTextViewAsync();
 
 			if (textView == null)
@@ -130,13 +132,13 @@ private async System.Threading.Tasks.Task CommandCallbackAsync()
 
 			// have to format, because cannot save all original indention
 			BqlFormatter formatter = BqlFormatter.FromTextView(textView);
-			var formatedRoot = formatter.Format(newSyntaxRoot, newSemanticModel);
+			var formattedRoot = formatter.Format(newSyntaxRoot, newSemanticModel);
 
-			await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync(); // Return to UI thread
+			await AcuminatorVSPackage.JTF.SwitchToMainThreadAsync(); // Return to UI thread
 
 			if (!textView.TextBuffer.EditInProgress)
 			{
-				var formattedDocument = document.WithSyntaxRoot(formatedRoot);
+				var formattedDocument = document.WithSyntaxRoot(formattedRoot);
 				ApplyChanges(document, formattedDocument);
 			}
 		}