Adding Lock statements to ProgressPane Hide and Show functions. #17498

pyrostew · 2022-06-08T09:26:38Z

PR Summary

Following the issue #17497 locks are being added to the Hide and Show methods in ProgressPane.cs to ensure that _savedRegion is not set to null after being check in the IsShowing property and being used in for loop in the Hide Method.

I am not sure if this is considered an acceptable fix for this issue, there might be a higher level that this would be better fixed at, but I'm not familiar enough with the code to know where that might be.

PR Context

See Issue #17497

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
Breaking changes
- None

ghost · 2022-06-08T09:26:50Z

All CLA requirements met.

ghost · 2022-06-16T14:01:19Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

sandscap-sc · 2022-07-20T15:55:17Z

@anmenaga @daxian-dbw @PaulHigin Can this be reviewed please?

FYI - I received the following in VSCode debugging a script that was using a lot of progress bars in the PS Integrated Terminal.

System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.PowerShell.ProgressPane.Hide()
   at Microsoft.PowerShell.ConsoleTextWriter.WriteLine(String value)
   at Microsoft.PowerShell.ConsoleHostUserInterface.WriteImpl(String value, Boolean newLine)
   at System.Management.Automation.MshCommandRuntime.WriteInformation(InformationRecord record, Boolean overrideInquire)
   at Microsoft.PowerShell.Commands.ReceiveJobCommand.WriteJobResults(Job job)
   at Microsoft.PowerShell.Commands.ReceiveJobCommand.WriteJobResultsRecursivelyHelper(Hashtable duplicate, Job job, Boolean registerInsteadOfWrite)
   at Microsoft.PowerShell.Commands.ReceiveJobCommand.WriteResultsForJobsInCollection(List`1 jobs, Boolean checkForRecurse, Boolean registerInsteadOfWrite)
   at Microsoft.PowerShell.Commands.ReceiveJobCommand.ProcessRecord()
   at System.Management.Automation.CommandProcessor.ProcessRec
8000
ord()

SteveL-MSFT

Other than the 1 suggestion, the change looks fine to me

SteveL-MSFT · 2022-07-25T20:09:40Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressPane.cs

@@ -26,7 +26,8 @@ class ProgressPane
        internal
        ProgressPane(ConsoleHostUserInterface ui)
        {
-            if (ui == null) throw new ArgumentNullException(nameof(ui));
+            if (ui == null)
+                throw new ArgumentNullException(nameof(ui));


Can't tell in this editor if it lines up right, but please add braces

Suggested change

throw new ArgumentNullException(nameof(ui));

{

throw new ArgumentNullException(nameof(ui));

}

daxian-dbw · 2022-07-25T23:04:18Z

@SteveL-MSFT In what cases would Hide be called from multiple threads?

iSazonov · 2022-07-26T03:26:53Z

My doubts are there are already locks in some code paths and we can fall in dead lock. I'd want understand what is a code path for the PR issue which bypasses existing locks.

PaulHigin

I don't know this code very well, but I am uncomfortable placing large sections of code within lock statements. My concern is causing a regression where existing scripts start dead locking.

If the goal is to protect internal state (_savedRegion, _progressRegion), then I feel the fix should focus more narrowly on that.

pyrostew · 2022-07-26T16:22:11Z

I believe these locks are a pretty safe addition. The state of the class is fully internal, the locks could only be deadlocked if something outside of the class managed to get a lock on the lock object, but the lock object is private so that isn't possible. Sprinkling more smaller locking objects around just the internal state objects would increase the risk of mistakes and make it harder to maintain going forward. The objective is to only allow the one of the hide and show methods to be called at any one time. The only way to get a deadlock from the added locks would be for one of the show/hide methods to call the other from inside the locked code block, which isn't happening and should be easy to spot in the future should further changes be made to these methods.

This was the simplest fix to an issue my team and I feel is pretty critical, clearly the ideal fix would be dig through the call tree and find where there is actually an issues with multiple threads interacting with the progress bars. but given the very specific scenario where I encountered this, it would be very complex to debug and find the true root cause.

daxian-dbw · 2022-07-26T16:54:57Z

I have the similar concern as @PaulHigin and @iSazonov.

By looking at the code, it seems the issue would happen only if more than one threads are calling Hide on the same ProgressPane instance -- one sets _savedRegion to null while the other is still at the early phase of the method.

The part that I don't understand is why there are multiple threads manipulating the same progress pane, and that feels to me is the root cause of this issue.

iSazonov · 2022-07-26T17:22:39Z

The part that I don't understand is why there are multiple threads manipulating the same progress pane, and that feels to me is the root cause of this issue.

My understanding always was that ConsoleHost is owner. And it has already has locks to exclude race conditions. But the issue shows there is a code path which bypasses these locks.

pyrostew · 2022-07-27T08:28:24Z

I'm happy to update this PR with suggestions for a more appropriate fix, but I'll need pointing at the right place as I'm not familiar with the architecture of the project.

iSazonov · 2022-07-28T06:08:43Z

I'm happy to update this PR with suggestions for a more appropriate fix, but I'll need pointing at the right place as I'm not familiar with the architecture of the project.

You could look locks which works for local scenario. There are two places with such locks if I remember right. Then it would be great to find where the code is called in remoting code. There is a deserialization for ProgressRecord - a root of the issue there.

PaulHigin · 2022-07-28T15:54:05Z

The WriteProgress call from the remoting layer is likely through the RemoteHost object handling on the client.

PowerShell/src/System.Management.Automation/engine/remoting/common/WireDataFormat/RemoteHost.cs

Line 136 in b780a34

internal static RemoteHostCall Decode(PSObject data)

PowerShell/src/System.Management.Automation/engine/remoting/host/RemoteHostMethodInfo.cs

Line 136 in b780a34

internal static RemoteHostMethodInfo LookUp(RemoteHostMethodId methodId)

The client normally marshals the remote host write onto the pipeline thread, and we should find out why that isn't happening in this case.

PowerShell/src/System.Management.Automation/engine/remoting/client/ClientMethodExecutor.cs

Line 71 in b780a34

internal static void Dispatch(

ghost · 2022-08-05T02:01:15Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

PaulHigin · 2022-08-08T17:18:21Z

Looking at the original issue (#17497), I see that this only occurs during a WinRM remoting disconnect/reconnect event. It appears that after reconnection, the remote host calls are no longer marshaled onto the client cmdlet thread. I would like to first understand why this is happening before adding locks. My guess is some client state information is lost after the reconnect.

ghost · 2022-08-16T02:01:18Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

pull-request-quantifier-deprecated · 2023-08-29T12:51:50Z

This PR has 119 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Medium
Size       : +61 -58
Percentile : 43.8%

Total files changed: 1

Change summary by file extension:
.cs : +61 -58

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

pyrostew · 2023-08-30T16:03:13Z

Hello All, @TravisEz13 @adityapatwardhan

Reawakening this one as I've just run headlong into it again on a newer version of PowerShell. I've rebased the original commit on master.

To make a point on some of the comments, it seems the feeling is that there is some issue further up the chain relative to the locks I've put in. I would argue that where I have put the locks in should have had them anyway. Whatever other issues exist, fundamentally there is a concurrent access issue in the ProgressPane class, and these changes fix that. If that means another bug becomes an non issue then isn't that win win?

I have had a go at trying to find the higher level issue but have found that I'm putting much more wide sweeping changes in to deal with it.

iSazonov

I believe there must be at least two threads and most likely with remote sessions to reproduce the problem (one ends its progress bar, second continues its progress bar). In this case this fix is not completely correct.

microsoft-github-policy-service · 2023-09-14T19:20:04Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

pyrostew requested review from daxian-dbw and anmenaga as code owners June 8, 2022 09:26

ghost assigned anmenaga Jun 8, 2022

pyrostew force-pushed the Issue17497 branch 2 times, most recently from a9fa194 to 7e98bfe Compare June 8, 2022 10:01

pull-request-quantifier-deprecated bot added the Medium label Jun 8, 2022

iSazonov requested a review from PaulHigin June 9, 2022 05:02

ghost added the Review - Needed The PR is being reviewed label Jun 16, 2022

iSazonov requested a review from SeeminglyScience July 21, 2022 03:42

SteveL-MSFT approved these changes Jul 25, 2022

View reviewed changes

ghost removed the Review - Needed The PR is being reviewed label Jul 25, 2022

PaulHigin reviewed Jul 26, 2022

View reviewed changes

ghost added the Review - Needed The PR is being reviewed label Aug 5, 2022

ghost removed the Review - Needed The PR is being reviewed label Aug 8, 2022

ghost added the Review - Needed The PR is being reviewed label Aug 16, 2022

adityapatwardhan changed the base branch from release/v7.2.5 to master September 27, 2022 19:53

adityapatwardhan requested review from TravisEz13 and adityapatwardhan as code owners September 27, 2022 19:53

pyrostew force-pushed the Issue17497 branch 2 times, most recently from 711e347 to 2e2426d Compare August 29, 2023 12:48

Adding Lock statements to ProgressPane Hide and Show functions.

77802a7

pyrostew force-pushed the Issue17497 branch from 2e2426d to 77802a7 Compare August 29, 2023 12:51

iSazonov requested changes Aug 30, 2023

View reviewed changes

microsoft-github-policy-service bot added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed labels Aug 30, 2023

microsoft-github-policy-service bot added the Stale label Sep 14, 2023

microsoft-github-policy-service bot closed this Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Lock statements to ProgressPane Hide and Show functions. #17498

Adding Lock statements to ProgressPane Hide and Show functions. #17498

What can I do to optimize my changes

How to interpret the change counts in git diff output

Adding Lock statements to ProgressPane Hide and Show functions. #17498

Adding Lock statements to ProgressPane Hide and Show functions. #17498

Conversation

PR Summary

PR Context

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

What can I do to optimize my changes

How to interpret the change counts in git diff output

Choose a reason for hiding this comment