8000 fix race condition between ProgressRecord being updated while rendering by SteveL-MSFT · Pull Request #2771 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

fix race condition between ProgressRecord being updated while rendering #2771

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

fix race condition between ProgressRecord being updated while rendering #2771

wants to merge 5 commits into from

Conversation

SteveL-MSFT
Copy link
Member

addresses #2756

@SteveL-MSFT
Copy link
Member Author

running tests under codecoverage is blocked by this, with this change, codecoverage run completed

@PowerShellTeam PowerShellTeam added the Review - Needed The PR is being reviewed label Nov 23, 2016
vors
vors reviewed Nov 23, 2016
View reviewed changes
src/Microsoft.PowerShell.ConsoleHost/host/msh/PendingProgress.cs
@@ -1207,8 +1216,6 @@ internal static
private ArrayList _topLevelNodes = new ArrayList();
private int _nodeCount;
private const int maxNodeCount = 128;
private static object _pendingProgressLock = new object();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why static? All affected methods are instance methods.

8000 Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

vors
vors approved these changes Nov 27, 2016
View reviewed changes
Copy link
Collaborator
@vors vors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@vors
Copy link
Collaborator
vors commented Nov 27, 2016

Actually, if RenderHelper is a hot-spot, read-write lock would be better here.

Also the comment at the top of the file suggests that class code could be simplified, but it's out of scope of this PR

/// This class uses lots of nearly identical helper functions to recursively traverse the tree. If I weren't so pressed 
/// for time, I would see if generic methods could be used to collapse the number of traversers.

@SteveL-MSFT
Copy link
Member Author

@vors made change to ReaderWriterLockSlim, verifying change

@iSazonov iSazonov mentioned this pull request Nov 29, 2016
Closed
@daxian-dbw
Copy link
Member
daxian-dbw commented Nov 29, 2016 •
edited
Loading

By reading the code, it looks to me that RenderHelper will only be called through ProgressPane.Show, and ProgressPane.Show will never be called from 2 different threads simultaneously -- it's either being called on the pipeline thread, or called from a timer thread, and only one timer thread will be running at any given time. That means at any given time, only 1 thread will be executing RenderHelper method, so a read-write lock won't help improve concurrency.

Second, this lock protects accessing to the type field _topLevelNodes, but there are other type members also shared between the main pipeline thread and _progPane.Show running on the timer thread, _rawui and _progressRegion are another 2 examples. I didn't do thorough analysis to know if those shared members can actually cause other race conditions, but it's possible.

Maybe we should just have a lock in ConsoleHostUserInterfaceProgress.cs to protect the call to _pendingProgress.Update(sourceId, record) in HandleIncomingProgressRecord and the call to _progPane.Show(_pendingProgress) in ProgressPaneUpdateTimerElapsed. I think that will cause very minimum performance degradation to the timer improvement.

@lzybkr @iSazonov Any thoughts?

@lzybkr
Copy link
Contributor
lzybkr commented Nov 29, 2016

If we are manipulating data structures as part of showing progress, maybe a timer was the wrong approach - it seems possible that we might make a mess of things somehow by not rendering everything that was asked of us.

On the other hand, maybe the progress rendering handles such cases already on the assumption that the user did a poor job, e.g. not reporting the completion of some child event.

@daxian-dbw
Copy link
Member

It turns out the current timer approach could result in more potential race conditions, for example, the timer might be disposed on the main pipeline thread while it's being called on the timer thread, the _progPane.Hide() might be called in ResetProgress on the main pipeline thread, while _progPane.Show is running on the timer thread. After talking to @SteveL-MSFT and @lzybkr, we feel the safest fix for now (to unblock CI/daily build) is to revert the original timer change. I will re-open the original Write-Progress issue too. /cc @iSazonov

I will close this PR and submit another one to revert the timer change.

@daxian-dbw daxian-dbw closed this Nov 29, 2016
@daxian-dbw daxian-dbw mentioned this pull request Nov 30, 2016
Closed
@daxian-dbw
Copy link
Member

The new PR is #2806.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vors vors vors approved these changes

Assignees
No one assigned
Labels
Review - Needed The PR is being reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@SteveL-MSFT @vors @daxian-dbw @lzybkr @PowerShellTeam @msftclas
0