Merge pull request #1975 from PowerShell/andschwa/debugger

Fix several bugs in the debugger

Author: andyleejordan

src/PowerShellEditorServices.Hosting/Commands/StartEditorServicesCommand.cs

@@ -15,7 +15,6 @@
 #if DEBUG
 using System.Diagnostics;
 using System.Threading;
-
 using Debugger = System.Diagnostics.Debugger;
 #endif
 
@@ -197,9 +196,11 @@ protected override void BeginProcessing()
 #if DEBUG
             if (WaitForDebugger)
             {
+                // NOTE: Ignore the suggestion to use Environment.ProcessId as it doesn't work for
+                // .NET 4.6.2 (for Windows PowerShell), and this won't be caught in CI.
+                Console.WriteLine($"Waiting for debugger to attach, PID: {Process.GetCurrentProcess().Id}");
                 while (!Debugger.IsAttached)
                 {
-                    Console.WriteLine($"PID: {Process.GetCurrentProcess().Id}");
                     Thread.Sleep(1000);
                 }
             }

src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs

@@ -241,11 +241,12 @@ public async Task<CommandBreakpointDetails[]> SetCommandBreakpointsAsync(
         /// that is identified by the given referenced ID.
         /// </summary>
         /// <param name="variableReferenceId"></param>
+        /// <param name="cancellationToken"></param>
         /// <returns>An array of VariableDetails instances which describe the requested variables.</returns>
-        public VariableDetailsBase[] GetVariables(int variableReferenceId)
+        public async Task<VariableDetailsBase[]> GetVariables(int variableReferenceId, CancellationToken cancellationToken)
         {
             VariableDetailsBase[] childVariables;
-            debugInfoHandle.Wait();
+            await debugInfoHandle.WaitAsync(cancellationToken).ConfigureAwait(false);
             try
             {
                 if ((variableReferenceId < 0) || (variableReferenceId >= variables.Count))
@@ -257,7 +258,12 @@ public VariableDetailsBase[] GetVariables(int variableReferenceId)
                 VariableDetailsBase parentVariable = variables[variableReferenceId];
                 if (parentVariable.IsExpandable)
                 {
-                    childVariables = parentVariable.GetChildren(_logger);
+                    // We execute this on the pipeline thread so the expansion of child variables works.
+                    childVariables = await _executionService.ExecuteDelegateAsync(
+                        $"Getting children of variable ${parentVariable.Name}",
+                        new ExecutionOptions { Priority = ExecutionPriority.Next },
+                        (_, _) => parentVariable.GetChildren(_logger), cancellationToken).ConfigureAwait(false);
+
                     foreach (VariableDetailsBase child in childVariables)
                     {
                         // Only add child if it hasn't already been added.
@@ -287,8 +293,9 @@ public VariableDetailsBase[] GetVariables(int variableReferenceId)
         /// walk the cached variable data for the specified stack frame.
         /// </summary>
         /// <param name="variableExpression">The variable expression string to evaluate.</param>
+        /// <param name="cancellationToken"></param>
         /// <returns>A VariableDetailsBase object containing the result.</returns>
-        public VariableDetailsBase GetVariableFromExpression(string variableExpression)
+        public async Task<VariableDetailsBase> GetVariableFromExpression(string variableExpression, CancellationToken cancellationToken)
         {
             // NOTE: From a watch we will get passed expressions that are not naked variables references.
             // Probably the right way to do this would be to examine the AST of the expr before calling
@@ -302,7 +309,7 @@ public VariableDetailsBase GetVariableFromExpression(string variableExpression)
             IEnumerable<VariableDetailsBase> variableList;
 
             // Ensure debug info isn't currently being built.
-            debugInfoHandle.Wait();
+            await debugInfoHandle.WaitAsync(cancellationToken).ConfigureAwait(false);
             try
             {
                 variableList = variables;
@@ -331,7 +338,7 @@ public VariableDetailsBase GetVariableFromExpression(string variableExpression)
                 if (resolvedVariable?.IsExpandable == true)
                 {
                     // Continue by searching in this variable's children.
-                    variableList = GetVariables(resolvedVariable.Id);
+                    variableList = await GetVariables(resolvedVariable.Id, cancellationToken).ConfigureAwait(false);
                 }
             }
 
@@ -491,18 +498,20 @@ public async Task<string> SetVariableAsync(int variableContainerReferenceId, str
         /// <param name="writeResultAsOutput">
         /// If true, writes the expression result as host output rather than returning the results.
         /// In this case, the return value of this function will be null.</param>
+        /// <param name="cancellationToken"></param>
         /// <returns>A VariableDetails object containing the result.</returns>
         public async Task<VariableDetails> EvaluateExpressionAsync(
             string expressionString,
-            bool writeResultAsOutput)
+            bool writeResultAsOutput,
+            CancellationToken cancellationToken)
         {
             PSCommand command = new PSCommand().AddScript(expressionString);
             IReadOnlyList<PSObject> results;
             try
             {
                 results = await _executionService.ExecutePSCommandAsync<PSObject>(
                     command,
-                    CancellationToken.None,
+                    cancellationToken,
                     new PowerShellExecutionOptions { WriteOutputToHost = writeResultAsOutput, ThrowOnError = !writeResultAsOutput }).ConfigureAwait(false);
             }
             catch (Exception e)

src/PowerShellEditorServices/Services/DebugAdapter/Debugging/VariableDetails.cs

@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation.
+// Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
 using System;
@@ -68,7 +68,7 @@ public VariableDetails(PSObject psVariableObject)
         /// The PSPropertyInfo instance from which variable details will be obtained.
         /// </param>
         public VariableDetails(PSPropertyInfo psProperty)
-            : this(psProperty.Name, psProperty.Value)
+            : this(psProperty.Name, SafeGetValue(psProperty))
         {
         }
 
@@ -98,16 +98,11 @@ public VariableDetails(string name, object value)
         /// If this variable instance is expandable, this method returns the
         /// details of its children.  Otherwise it returns an empty array.
         /// </summary>
-        /// <returns></returns>
         public override VariableDetailsBase[] GetChildren(ILogger logger)
         {
             if (IsExpandable)
             {
-                if (cachedChildren == null)
-                {
-                    cachedChildren = GetChildren(ValueObject, logger);
-                }
-
+                cachedChildren ??= GetChildren(ValueObject, logger);
                 return cachedChildren;
             }
 
@@ -118,6 +113,20 @@ public override VariableDetailsBase[] GetChildren(ILogger logger)
 
         #region Private Methods
 
+        private static object SafeGetValue(PSPropertyInfo psProperty)
+        {
+            try
+            {
+                return psProperty.Value;
+            }
+            catch (GetValueInvocationException ex)
+            {
+                // Sometimes we can't get the value, like ExitCode, for reasons beyond our control,
+                // so just return the message from the exception that arises.
+                return new UnableToRetrievePropertyMessage { Name = psProperty.Name, Message = ex.Message };
+            }
+        }
+
         private static bool GetIsExpandable(object valueObject)
         {
             if (valueObject == null)
@@ -131,9 +140,7 @@ private static bool GetIsExpandable(object valueObject)
                 valueObject = psobject.BaseObject;
             }
 
-            Type valueType =
-                valueObject?.GetType();
-
+            Type valueType = valueObject?.GetType();
             TypeInfo valueTypeInfo = valueType.GetTypeInfo();
 
             return
@@ -236,7 +243,7 @@ private static string InsertDimensionSize(string value, int dimensionSize)
             return value + ": " + dimensionSize;
         }
 
-        private VariableDetails[] GetChildren(object obj, ILogger logger)
+        private static VariableDetails[] GetChildren(object obj, ILogger logger)
         {
             List<VariableDetails> childVariables = new();
 
@@ -245,86 +252,82 @@ private VariableDetails[] GetChildren(object obj, ILogger logger)
                 return childVariables.ToArray();
             }
 
-            try
-            {
-                PSObject psObject = obj as PSObject;
+            // NOTE: Variable expansion now takes place on the pipeline thread as an async delegate,
+            // so expansion of children that cause PowerShell script code to execute should
+            // generally work. However, we might need more error handling.
+            PSObject psObject = obj as PSObject;
 
-                if ((psObject != null) &&
-                    (psObject.TypeNames[0] == typeof(PSCustomObject).ToString()))
+            if ((psObject != null) &&
+                (psObject.TypeNames[0] == typeof(PSCustomObject).ToString()))
+            {
+                // PowerShell PSCustomObject's properties are completely defined by the ETS type system.
+                logger.LogDebug("PSObject was a PSCustomObject");
+                childVariables.AddRange(
+                    psObject
+                        .Properties
+                        .Select(p => new VariableDetails(p)));
+            }
+            else
+            {
+                // If a PSObject other than a PSCustomObject, unwrap it.
+                if (psObject != null)
                 {
-                    // PowerShell PSCustomObject's properties are completely defined by the ETS type system.
+                    // First add the PSObject's ETS properties
+                    logger.LogDebug("PSObject was something else, first getting ETS properties");
                     childVariables.AddRange(
                         psObject
                             .Properties
+                            // Here we check the object's MemberType against the `Properties`
+                            // bit-mask to determine if this is a property. Hence the selection
+                            // will only include properties.
+                            .Where(p => (PSMemberTypes.Properties & p.MemberType) is not 0)
                             .Select(p => new VariableDetails(p)));
+
+                    obj = psObject.BaseObject;
                 }
-                else
-                {
-                    // If a PSObject other than a PSCustomObject, unwrap it.
-                    if (psObject != null)
-                    {
-                        // First add the PSObject's ETS properties
-                        childVariables.AddRange(
-                            psObject
-                                .Properties
-                                // Here we check the object's MemberType against the `Properties`
-                                // bit-mask to determine if this is a property. Hence the selection
-                                // will only include properties.
-                                .Where(p => (PSMemberTypes.Properties & p.MemberType) is not 0)
-                                .Select(p => new VariableDetails(p)));
-
-                        obj = psObject.BaseObject;
-                    }
 
-                    // We're in the realm of regular, unwrapped .NET objects
-                    if (obj is IDictionary dictionary)
+                // We're in the realm of regular, unwrapped .NET objects
+                if (obj is IDictionary dictionary)
+                {
+                    logger.LogDebug("PSObject was an IDictionary");
+                    // Buckle up kids, this is a bit weird.  We could not use the LINQ
+                    // operator OfType<DictionaryEntry>.  Even though R# will squiggle the
+                    // "foreach" keyword below and offer to convert to a LINQ-expression - DON'T DO IT!
+                    // The reason is that LINQ extension methods work with objects of type
+                    // IEnumerable.  Objects of type Dictionary<,>, respond to iteration via
+                    // IEnumerable by returning KeyValuePair<,> objects.  Unfortunately non-generic
+                    // dictionaries like HashTable return DictionaryEntry objects.
+                    // It turns out that iteration via C#'s foreach loop, operates on the variable's
+                    // type which in this case is IDictionary.  IDictionary was designed to always
+                    // return DictionaryEntry objects upon iteration and the Dictionary<,> implementation
+                    // honors that when the object is reinterpreted as an IDictionary object.
+                    // FYI, a test case for this is to open $PSBoundParameters when debugging a
+                    // function that defines parameters and has been passed parameters.
+                    // If you open the $PSBoundParameters variable node in this scenario and see nothing,
+                    // this code is broken.
+                    foreach (DictionaryEntry entry in dictionary)
                     {
-                        // Buckle up kids, this is a bit weird.  We could not use the LINQ
-                        // operator OfType<DictionaryEntry>.  Even though R# will squiggle the
-                        // "foreach" keyword below and offer to convert to a LINQ-expression - DON'T DO IT!
-                        // The reason is that LINQ extension methods work with objects of type
-                        // IEnumerable.  Objects of type Dictionary<,>, respond to iteration via
-                        // IEnumerable by returning KeyValuePair<,> objects.  Unfortunately non-generic
-                        // dictionaries like HashTable return DictionaryEntry objects.
-                        // It turns out that iteration via C#'s foreach loop, operates on the variable's
-                        // type which in this case is IDictionary.  IDictionary was designed to always
-                        // return DictionaryEntry objects upon iteration and the Dictionary<,> implementation
-                        // honors that when the object is reinterpreted as an IDictionary object.
-                        // FYI, a test case for this is to open $PSBoundParameters when debugging a
-                        // function that defines parameters and has been passed parameters.
-                        // If you open the $PSBoundParameters variable node in this scenario and see nothing,
-                        // this code is broken.
-                        foreach (DictionaryEntry entry in dictionary)
-                        {
-                            childVariables.Add(
-                                new VariableDetails(
-                                    "[" + entry.Key + "]",
-                                    entry));
-                        }
+                        childVariables.Add(
+                            new VariableDetails(
+                                "[" + entry.Key + "]",
+                                entry));
                     }
-                    else if (obj is IEnumerable enumerable and not string)
+                }
+                else if (obj is IEnumerable enumerable and not string)
+                {
+                    logger.LogDebug("PSObject was an IEnumerable");
+                    int i = 0;
+                    foreach (object item in enumerable)
                     {
-                        int i = 0;
-                        foreach (object item in enumerable)
-                        {
-                            childVariables.Add(
-                                new VariableDetails(
-                                    "[" + i++ + "]",
-                                    item));
-                        }
+                        childVariables.Add(
+                            new VariableDetails(
+                                "[" + i++ + "]",
+                                item));
                     }
-
-                    AddDotNetProperties(obj, childVariables);
                 }
-            }
-            catch (GetValueInvocationException ex)
-            {
-                // This exception occurs when accessing the value of a
-                // variable causes a script to be executed.  Right now
-                // we aren't loading children on the pipeline thread so
-                // this causes an exception to be raised.  In this case,
-                // just return an empty list of children.
-                logger.LogWarning($"Failed to get properties of variable {Name}, value invocation was attempted: {ex.Message}");
+
+                logger.LogDebug("Adding .NET properties to PSObject");
+                AddDotNetProperties(obj, childVariables);
             }
 
             return childVariables.ToArray();
@@ -342,9 +345,8 @@ protected static void AddDotNetProperties(object obj, List<VariableDetails> chil
                 return;
             }
 
-            PropertyInfo[] properties = objectType.GetProperties(BindingFlags.Public | BindingFlags.Instance);
-
-            foreach (PropertyInfo property in properties)
+            // Search all the public instance properties and add those missing.
+            foreach (PropertyInfo property in objectType.GetProperties(BindingFlags.Public | BindingFlags.Instance))
             {
                 // Don't display indexer properties, it causes an exception anyway.
                 if (property.GetIndexParameters().Length > 0)
@@ -354,10 +356,11 @@ protected static void AddDotNetProperties(object obj, List<VariableDetails> chil
 
                 try
                 {
-                    childVariables.Add(
-                        new VariableDetails(
-                            property.Name,
-                            property.GetValue(obj)));
+                    // Only add unique properties because we may have already added some.
+                    if (!childVariables.Exists(p => p.Name == property.Name))
+                    {
+                        childVariables.Add(new VariableDetails(property.Name, property.GetValue(obj)));
+                    }
                 }
                 catch (Exception ex)
                 {
@@ -371,21 +374,19 @@ protected static void AddDotNetProperties(object obj, List<VariableDetails> chil
                     childVariables.Add(
                         new VariableDetails(
                             property.Name,
-                            new UnableToRetrievePropertyMessage(
-                                "Error retrieving property - " + ex.GetType().Name)));
+                            new UnableToRetrievePropertyMessage { Name = property.Name, Message = ex.Message }));
                 }
             }
         }
 
         #endregion
 
-        private struct UnableToRetrievePropertyMessage
+        private record UnableToRetrievePropertyMessage
         {
-            public UnableToRetrievePropertyMessage(string message) => Message = message;
-
-            public string Message { get; }
+            public string Name { get; init; }
+            public string Message { get; init; }
 
-            public override string ToString() => "<" + Message + ">";
+            public override string ToString() => $"Error retrieving property '${Name}': ${Message}";
         }
     }
 

src/PowerShellEditorServices/Services/DebugAdapter/Debugging/VariableDetailsBase.cs

@@ -51,7 +51,6 @@ internal abstract class VariableDetailsBase
         /// If this variable instance is expandable, this method returns the
         /// details of its children.  Otherwise it returns an empty array.
         /// </summary>
-        /// <returns></returns>
         public abstract VariableDetailsBase[] GetChildren(ILogger logger);
     }
 }