AnalysisService improvments #1179

rjmholt · 2020-02-07T02:59:27Z

This PR was originally supposed to stop crashes when we can't find PSScriptAnalyzer on the module path (when the Create() method returns null), but instead does a few things:

Allows on-demand disabling of the AnalysisService when the configuration changes
Adds a ConfigurationChanged event so that things to can subscribe to it early on
Breaks the thing running the analysis out from the service delivering it to other parts of the codebase, meaning it will be easier to substitute in a hosted PSScriptAnalyzer engine later
Improves cancellation of script analysis, both in terms of the delay but also in terms of being able to cancel when the actual analysis request is cancelled now
Moves a some cohesion/coupling issues that were clearly in the domain of the AnalysisService behind its API:
- The configuration change thing
- Comment help generation details
Fixes a log call where we (1) claim not to handle diagnostics with long IDs (even though we can in the function that takes diagnostic IDs) and (2) process them anyway (no continue)...
Improves how we publish CodeActions:
- Formerly we would:
  - Receive a diagnostics request
  - Start processing this and eventually get to updating the dictionary
  - Take out locks for each file being updated in the dictionary
  - Update the dictionary for each file
  - And then the caller would have to acquire a lock per file
  - Then copy a whole dictionary of things into their context
  - The worst part being that the CodeAction request would usually come before the diagnostic task had passed its delay
- So now, we ditch the locks and instead:
  - Diagnostic requests set up tasks and register them to each file
  - When a code action request comes through, it waits for the associated task to finish
  - Then it just reads directly out of the concurrent dictionary
  - (This isn't guaranteed to be a clean read, but it's likely that it will be and if it's not it's because more diagnostic requests are happening because the file's being updated -- so it doesn't mean much to not be clean)
Oh, I also seem to have fixed a URI encoding issue on Windows, but we could probably fix it better and in a more universal way

Aids PowerShell/vscode-powershell#2127 and PowerShell/vscode-powershell#2190

rjmholt · 2020-02-07T03:05:45Z

src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs

+            // Load any of the profile paths that exist
+            var command = new PSCommand();
+            bool hasLoadablePath = false;
+            foreach (var profilePath in GetLoadableProfilePaths(this.profilePaths))


Oh, I forgot. I was seeing an exception here in the debugger because none of the profile files existed. And it was my change that caused it in another PR. So I fixed it.

TylerLeonhardt · 2020-02-11T17:59:10Z

src/PowerShellEditorServices/Services/Analysis/AnalysisService.cs

 using Microsoft.Extensions.Logging;
 using OmniSharp.Extensions.LanguageServer.Protocol.Models;
 using OmniSharp.Extensions.LanguageServer.Protocol.Server;
 using System.Threading;
 using System.Collections.Concurrent;
 using Microsoft.PowerShell.EditorServices.Services.TextDocument;
-using Microsoft.PowerShell.EditorServices.Utility;
+using Microsoft.PowerShell.EditorServices.Services.Analysis;
+using Microsoft.PowerShell.EditorServices.Services.Configuration;


nit: can you sort these?

don't forget this

TylerLeonhardt · 2020-02-11T18:03:15Z

src/PowerShellEditorServices/Services/Analysis/AnalysisService.cs

+            _workplaceService = workspaceService;
+            _analysisDelayMillis = 750;
+            _mostRecentCorrectionsByFile = new ConcurrentDictionary<string, CorrectionTableEntry>();
+            _hasInstantiatedAnalysisEngine = false;


potential threading concern... If you wanna go down this route, maybe it's best to make the AnalysisEngine Lazy<>

Yeah I was thinking about that, but unlike Lazy<>, this will respond to a configuration update by re-instantiating the engine. It might be that the best solution is to put a lock in the getter?

maybe use an Interlocked.Exchange or something instead. I'm concerned that if there's a configuration update when this is being instantiated for the first time that that could be a race condition?

TylerLeonhardt · 2020-02-11T18:03:18Z

src/PowerShellEditorServices/Services/Analysis/AnalysisService.cs

-            _pssaModuleInfo = pssaModuleInfo;
-            _mostRecentCorrectionsByFile = new ConcurrentDictionary<string, (SemaphoreSlim, Dictionary<string, MarkerCorrection>)>();
+            _workplaceService = workspaceService;
+            _analysisDelayMillis = 750;


can this be a constant instead?

I was wondering whether to put it in as a constructor parameter. It used to be inline where the task is. As a field, we could use reflection to change the setting even.

src/PowerShellEditorServices/Services/Analysis/AnalysisService.cs

src/PowerShellEditorServices/Services/TextDocument/Handlers/TextDocumentHandler.cs

TylerLeonhardt · 2020-02-11T18:39:24Z

src/PowerShellEditorServices/Services/Workspace/WorkspaceService.cs

@@ -530,7 +535,7 @@ internal string ResolveRelativeScriptPath(string baseFilePath, string relativePa
            return combinedPath;
        }

-        private static Uri UnescapeDriveColon(Uri fileUri)
+        internal static Uri UnescapeDriveColon(Uri fileUri)


revert this when you use the other API

TylerLeonhardt · 2020-02-11T18:40:33Z

test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs

            Diagnostics.Clear();

+            PwshExe = TestsFixture.PwshExe;


what's the reason for this change?

I needed to edit the tests and moved this down so it wasn't with the list instantiation code

src/PowerShellEditorServices/Services/Analysis/AnalysisService.cs

TylerLeonhardt · 2020-02-12T17:50:56Z

src/PowerShellEditorServices/Services/Analysis/PssaCmdletAnalysisEngine.cs

+        /// <returns>The output of PowerShell execution.</returns>
+        private Collection<PSObject> InvokePowerShellWithModulePathPreservation(System.Management.Automation.PowerShell powershell)
+        {
+            if (_alreadyRun)


What is this check for?

Since runspace creation only happens at first invocation, I put a boolean in to reduce the overhead on subsequent calls

Run a few manual tests to make sure - I know RunspacePool has a habit of creating new runspaces within the pool (as in, we don't have control over when, why and how it creates Runspaces). I've seen in Get-Runspace incremental runspace numbering which is why I have concerns here.

src/PowerShellEditorServices/Services/Analysis/PssaCmdletAnalysisEngine.cs

src/PowerShellEditorServices/Services/TextDocument/ScriptFileMarker.cs

test/PowerShellEditorServices.Test.E2E/LSPTestsFixures.cs

TylerLeonhardt · 2020-02-14T00:45:37Z

src/PowerShellEditorServices/Services/Analysis/AnalysisService.cs

 using System.Collections;
-using System.Runtime.InteropServices;
+using System.Collections.Concurrent;
+using System.Collections.Generic;


nit: usually system* come first, then sorted everything else.

That's how VS sorted it. I've corrected

TylerLeonhardt · 2020-02-14T00:45:50Z

src/PowerShellEditorServices/Services/Analysis/AnalysisService.cs


        private CancellationTokenSource _diagnosticsCancellationTokenSource;

+        #region Engine Initialization
+        #endregion


empty region?

TylerLeonhardt · 2020-02-14T00:47:17Z

src/PowerShellEditorServices/Services/Analysis/PssaCmdletAnalysisEngine.cs

@@ -107,6 +107,8 @@ public PssaCmdletAnalysisEngine Build()
            }
        }

+        private const int PSSA_RUNPOOL_SIZE = 10;


has this always been 10? 😮

TylerLeonhardt · 2020-02-14T00:48:55Z

src/PowerShellEditorServices/Services/Analysis/PssaCmdletAnalysisEngine.cs

-                _alreadyRun = true;
+                finally
+                {
+                    Interlocked.Increment(ref _pssaRunCount);


is it guaranteed that a RunspacePool uses a new Runspace until it hits the max of what it can support?

Otherwise the first 10 runs might use the same runspace and then subsequent runs spin up new runspaces.

TylerLeonhardt

LGTM

Rob Holt and others added 17 commits February 6, 2020 18:46

Fix AnalysisService initialization

2710077

Remove old analysis service

104da3f

Restore old runspace pool settings

617ba31

Change analysis engine throwing to null analysis engine

ba36e45

Add some comments

d3d22a9

Allow AnalysisService to reinstantiate its engine

ccb4caa

Move more functionality behind AnalysisService

660e895

Move help API into AnalysisService

e3dee1a

Add comments

0a488c2

Simplify design and add more comments

0b10f9a

Fix logger instantiation

03fdc3c

Undo new switch syntax

da2491c

Fix code action out-of-order issue

5154ed7

Fix codeAction issue

062ca75

Add comment about URI fix

983b3bb

Remove null diagnostics from test

c9edb7a

Remove unused imports

62fd954

rjmholt requested review from TylerLeonhardt, rkeithhill and SeeminglyScience February 7, 2020 02:59

rjmholt added 2 commits February 6, 2020 19:04

Explicitly get PssaCmdletAnalysisEngine to implement IDisposable

a0d006c

Add ConfigureAwait(false)

92d79b7

rjmholt commented Feb 7, 2020

View reviewed changes

rjmholt added 2 commits February 6, 2020 19:14

Remove commented out code

7c0cee5

Better event invocation

a4e65ad

PowerShell deleted a comment from rjmholt Feb 7, 2020

rjmholt mentioned this pull request Feb 10, 2020

Check _analysisService is null #1175

Closed

TylerLeonhardt reviewed Feb 11, 2020

View reviewed changes

Change codeaction dict to use scriptfiles

5d6cf78

PowerShell deleted a comment from rjmholt Feb 11, 2020

Address @TylerLeonhardt's comments

2aed0a6

TylerLeonhardt reviewed Feb 12, 2020

View reviewed changes

src/PowerShellEditorServices/Services/Analysis/AnalysisService.cs Show resolved Hide resolved

TylerLeonhardt reviewed Feb 12, 2020

View reviewed changes

src/PowerShellEditorServices/Services/Analysis/PssaCmdletAnalysisEngine.cs Show resolved Hide resolved

TylerLeonhardt reviewed Feb 12, 2020

View reviewed changes

src/PowerShellEditorServices/Services/TextDocument/ScriptFileMarker.cs Outdated Show resolved Hide resolved

TylerLeonhardt reviewed Feb 12, 2020

View reviewed changes

test/PowerShellEditorServices.Test.E2E/LSPTestsFixures.cs Outdated Show resolved Hide resolved

Rob Holt added 6 commits February 13, 2020 13:47

Ensure PSModulePath is preserved when looking for PSSA

47c8b0b

Remove unused method

81267d1

Remove unneeded using

393bd59

Add comment explaining correction table entry class

da558b4

Enhance PSSA ISS comment

7dd68e2

Address issues

23dbbe3

TylerLeonhardt reviewed Feb 14, 2020

View reviewed changes

Rob Holt added 4 commits February 13, 2020 17:15

Remove cleverness with PSSA runspace management

de071f9

Remove unused constant

2d7fd1d

Remove unused region

c9b2a4c

Change using order

e3dbcd4

TylerLeonhardt approved these changes Feb 14, 2020

View reviewed changes

rjmholt merged commit c99aae3 into PowerShell:master Feb 14, 2020

TylerLeonhardt mentioned this pull request Feb 24, 2020

Make PSSA look for a file in the workspace root by default PowerShell/vscode-powershell#2484

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AnalysisService improvments #1179

AnalysisService improvments #1179

rjmholt commented Feb 7, 2020 •

edited

Loading

rjmholt Feb 7, 2020

TylerLeonhardt Feb 11, 2020

TylerLeonhardt Feb 12, 2020

TylerLeonhardt Feb 11, 2020

rjmholt Feb 11, 2020

TylerLeonhardt Feb 13, 2020

TylerLeonhardt Feb 11, 2020

rjmholt Feb 11, 2020

TylerLeonhardt Feb 11, 2020

TylerLeonhardt Feb 11, 2020

rjmholt Feb 11, 2020

TylerLeonhardt Feb 12, 2020

rjmholt Feb 13, 2020

TylerLeonhardt Feb 13, 2020

TylerLeonhardt Feb 14, 2020

rjmholt Feb 14, 2020

TylerLeonhardt Feb 14, 2020

TylerLeonhardt Feb 14, 2020

TylerLeonhardt Feb 14, 2020

TylerLeonhardt Feb 14, 2020

TylerLeonhardt left a comment

AnalysisService improvments #1179

AnalysisService improvments #1179

Conversation

rjmholt commented Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt left a comment

Choose a reason for hiding this comment

rjmholt commented Feb 7, 2020 •

edited

Loading