Fix TEMP debugging #1065

TylerLeonhardt · 2019-10-11T11:44:57Z

This isn't quite ready yet. But I wanted to share what it took.

src/PowerShellEditorServices/Services/DebugAdapter/Handlers/ConfigurationDoneHandler.cs

TylerLeonhardt · 2019-10-12T00:53:27Z

NOTE: This last commit requires a change in PSRL.

TylerLeonhardt · 2019-10-14T15:26:47Z

Marking as ready for review but it does need that PSRL change to go in.

rjmholt · 2019-10-14T18:13:32Z

src/PowerShellEditorServices/Hosting/EditorServicesHost.cs

@@ -291,6 +292,25 @@ public void StartLogging(string logFilePath, PsesLogLevel logLevel)
        {
            _logger.LogInformation($"Debug NamedPipe: {config.InOutPipeName}\nDebug OutPipe: {config.OutPipeName}");

+            IServiceProvider serviceProvider = null;
+            if (!useExistingSession)


Is there a reason for not using useTempSession with inverted logic? That would seem to line up with the conceptual feature better to me, and feels like it aligns with "breaking out" of the normal code when the useTempSession switch is set.

Yeah that's fine - useExistingSession was just "what was there before"

changed it to useTempSession hopefully that's a bit more clear.

src/PowerShellEditorServices/Hosting/EditorServicesHost.cs

rjmholt · 2019-10-14T18:35:13Z

src/PowerShellEditorServices/Server/PsesDebugServer.cs

@@ -34,9 +36,10 @@ public class PsesDebugServer : IDisposable
            _loggerFactory = factory;
            _inputStream = inputStream;
            _outputStream = outputStream;
+            _serverStart = new TaskCompletionSource<bool>();


Suggested change

_serverStart = new TaskCompletionSource<bool>();

_serverStarted = new TaskCompletionSource<bool>();

gonna change this to _serverRunning as it will be used to "wait until the server is not running"

changed it to that because when the "result is set" it's like saying set the result of the "running". Let me know if you feel stronger about _serverStopped

rjmholt · 2019-10-14T18:36:12Z

src/PowerShellEditorServices/Server/PsesDebugServer.cs

+
+        public async Task WaitForShutdown()
+        {
+            await _serverStart.Task;


If this is server start, why does it get set on shutdown? Should it be something like serverStopped instead?

Yeah I guess it should be called like _serverRunning or something.

changed it to that because when the "result is set" it's like saying set the result of the "running". Let me know if you feel stronger about _serverStopped

I'd personally expect _serverRunning to complete after initialization. I'm good with _serverStopped, _serverSession, _shutdownRequested anything like that.

Changed to _serverStopped!

rjmholt · 2019-10-14T18:37:20Z

src/PowerShellEditorServices/Server/PsesServiceCollection.cs

+
+namespace Microsoft.PowerShell.EditorServices.Server
+{
+    internal static class PsesServiceCollection


Given that this is a static extensions class, maybe call it something like PsesServiceCollectionExtensions instead (i.e. it doesn't actually implement an collection of services)

TylerLeonhardt · 2019-10-14T19:22:24Z

build might fail for this because of needed changes in #1066

but I've addressed all your feedback @rjmholt

SeeminglyScience

Some of these comments might be outdated, started the review yesterday. If they're no longer applicable just skip em. Mostly nits, otherwise LGTM!

SeeminglyScience · 2019-10-15T18:50:33Z

src/PowerShellEditorServices/Server/PsesDebugServer.cs

+
+        public async Task WaitForShutdown()
+        {
+            await _serverStart.Task;


I'd personally expect _serverRunning to complete after initialization. I'm good with _serverStopped, _serverSession, _shutdownRequested anything like that.

SeeminglyScience · 2019-10-15T18:59:54Z

src/PowerShellEditorServices/Server/PsesServiceCollection.cs

+            IServiceProvider languageServiceProvider,
+            PsesDebugServer psesDebugServer,
+            bool useExistingSession
+        )


nit: move close paren to previous line

SeeminglyScience · 2019-10-15T19:00:06Z

src/PowerShellEditorServices/Server/PsesServiceCollection.cs

+                    });
+        }
+
+        public static IServiceCollection AddPsesDebugServices (


Suggested change

public static IServiceCollection AddPsesDebugServices (

public static IServiceCollection AddPsesDebugServices(

SeeminglyScience · 2019-10-15T19:10:01Z

src/PowerShellEditorServices/Hosting/EditorServicesHost.cs

-            _languageServer.WaitForShutdown().Wait();
+            if (_languageServer != null)
+            {
+                _languageServer.WaitForShutdown().Wait();


If WaitForShutdown returns a Task we should use GetAwaiter().GetResult() instead of Wait or Result. The latter two wrap exceptions in a AggregateException with a generic message and messes with the stack trace a bit (or rather doesn't fix it).

I know it's existing code, just something we should probably clean up as we see it.

TylerLeonhardt · 2019-10-16T19:10:08Z

All feedback addressed and rebased! The can be merged in really at any time it just won't work until PSRL is released.

TylerLeonhardt · 2019-10-28T16:43:18Z

This is not being backported.

TylerLeonhardt commented Oct 11, 2019

View reviewed changes

src/PowerShellEditorServices/Services/DebugAdapter/Handlers/ConfigurationDoneHandler.cs Show resolved Hide resolved

PowerShell deleted a comment Oct 12, 2019

TylerLeonhardt marked this pull request as ready for review October 14, 2019 15:26

TylerLeonhardt requested a review from rjmholt as a code owner October 14, 2019 15:26

rjmholt reviewed Oct 14, 2019

View reviewed changes

rjmholt approved these changes Oct 14, 2019

View reviewed changes

PowerShell deleted a comment Oct 15, 2019

TylerLeonhardt added 5 commits October 15, 2019 22:32

refactor out servicecollection additions

021a147

initial working TEMP debugging

e8c7ad9

this makes me cry

8024e0b

wait for completion of script

ad584c0

address rob's feedback

c369003

SeeminglyScience approved these changes Oct 16, 2019

View reviewed changes

address Patrick's feedback

598bf16

TylerLeonhardt force-pushed the add-debug-tests branch from 0fcf4d2 to 598bf16 Compare October 16, 2019 18:55

TylerLeonhardt merged commit aab80d1 into PowerShell:master Oct 28, 2019

TylerLeonhardt deleted the add-debug-tests branch October 28, 2019 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TEMP debugging #1065

Fix TEMP debugging #1065

TylerLeonhardt commented Oct 11, 2019

TylerLeonhardt commented Oct 12, 2019 •

edited

Loading

TylerLeonhardt commented Oct 14, 2019

rjmholt Oct 14, 2019

TylerLeonhardt Oct 14, 2019

TylerLeonhardt Oct 14, 2019

rjmholt Oct 14, 2019

TylerLeonhardt Oct 14, 2019 •

edited

Loading

TylerLeonhardt Oct 14, 2019

rjmholt Oct 14, 2019

TylerLeonhardt Oct 14, 2019

TylerLeonhardt Oct 14, 2019

SeeminglyScience Oct 15, 2019

TylerLeonhardt Oct 16, 2019

rjmholt Oct 14, 2019

TylerLeonhardt commented Oct 14, 2019

SeeminglyScience left a comment

SeeminglyScience Oct 15, 2019

SeeminglyScience Oct 15, 2019

TylerLeonhardt Oct 16, 2019

SeeminglyScience Oct 15, 2019

TylerLeonhardt Oct 16, 2019

SeeminglyScience Oct 15, 2019

TylerLeonhardt Oct 16, 2019

TylerLeonhardt commented Oct 16, 2019

TylerLeonhardt commented Oct 28, 2019

	_serverStart = new TaskCompletionSource<bool>();
	_serverStarted = new TaskCompletionSource<bool>();

	public static IServiceCollection AddPsesDebugServices (
	public static IServiceCollection AddPsesDebugServices(

Fix TEMP debugging #1065

Fix TEMP debugging #1065

Conversation

TylerLeonhardt commented Oct 11, 2019

TylerLeonhardt commented Oct 12, 2019 • edited Loading

TylerLeonhardt commented Oct 14, 2019

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 Oct 14, 2019 • 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

TylerLeonhardt commented Oct 14, 2019

SeeminglyScience left a 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 commented Oct 16, 2019

TylerLeonhardt commented Oct 28, 2019

TylerLeonhardt commented Oct 12, 2019 •

edited

Loading

TylerLeonhardt Oct 14, 2019 •

edited

Loading