Skip to content

Conversation

@jianyunt
Copy link
Contributor

@jianyunt jianyunt commented Jun 8, 2018

PR Summary

Addressed the issue #7022
For the PSReadLine part of issue, see PR PowerShell/PSReadLine#708

PR Checklist

// Reset the cursor back to where it started
if(record.RecordType == ProgressRecordType.Completed)
{
_progPane.Hide();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We do the hide at a pipeline end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I observed that the hide() gets called when write-progress completes for cmdlets. However for the cloudshell case, the SHiPS provider calls write-progress when a user hits get tab for example; it then calls a Azure cmdlet that triggers write-progress too. When the Azure cmdlet completes, the hide() gets called. PowerShell handles it properly. But when the WriteProgress for ProgressRecordType.Completed gets called from the SHiPS provider, Show() gets called again. At that time, record.RecordType is ProgressRecordType.Completed. After that, no more hide() gets called.

image

Call stack:
Microsoft.PowerShell.ConsoleHost.dll!Microsoft.PowerShell.ProgressPane.Show() Line 95
Microsoft.PowerShell.ConsoleHost.dll!Microsoft.PowerShell.ProgressPane.Show(Microsoft.PowerShell.PendingProgress pendingProgress) Line 250
Microsoft.PowerShell.ConsoleHost.dll!Microsoft.PowerShell.ConsoleHostUserInterface.HandleIncomingProgressRecord(long sourceId, System.Management.Automation.ProgressRecord record) Line 99
Microsoft.PowerShell.ConsoleHost.dll!Microsoft.PowerShell.ConsoleHostUserInterface.WriteProgress(long sourceId, System.Management.Automation.ProgressRecord record) Line 1324
System.Management.Automation.dll!System.Management.Automation.Internal.Host.InternalHostUserInterface.WriteProgress(long sourceId, System.Management.Automation.ProgressRecord record) Line 609
System.Management.Automation.dll!System.Management.Automation.MshCommandRuntime.WriteProgress(long sourceId, System.Management.Automation.ProgressRecord progressRecord, bool overrideInquire) Line 413
System.Management.Automation.dll!System.Management.Automation.MshCommandRuntime.WriteProgress(System.Management.Automation.ProgressRecord progressRecord, bool overrideInquire) Line 354
System.Management.Automation.dll!System.Management.Automation.MshCommandRuntime.WriteProgress(System.Management.Automation.ProgressRecord progressRecord) Line 324
System.Management.Automation.dll!System.Management.Automation.Cmdlet.WriteProgress(System.Management.Automation.ProgressRecord progressRecord) Line 560
System.Management.Automation.dll!System.Management.Automation.CmdletProviderContext.WriteProgress(System.Management.Automation.ProgressRecord record) Line 851
System.Management.Automation.dll!System.Management.Automation.Provider.CmdletProvider.WriteProgress(System.Management.Automation.ProgressRecord progressRecord) Line 1794
Microsoft.PowerShell.SHiPS.dll!Microsoft.PowerShell.SHiPS.ProgressTracker.End(CodeOwls.PowerShell.Provider.PathNodeProcessors.IProviderContext context) Line 56
Microsoft.PowerShell.SHiPS.dll!Microsoft.PowerShell.SHiPS.PSScriptRunner.InvokeScriptBlock(CodeOwls.PowerShell.Provider.PathNodeProcessors.IProviderContext context, Microsoft.PowerShell.SHiPS.SHiPSDirectory node, Microsoft.PowerShell.SHiPS.SHiPSDrive drive) Line 87
Microsoft.PowerShell.SHiPS.dll!Microsoft.PowerShell.SHiPS.ContainerNodeService.GetNodeChildrenInternal(CodeOwls.PowerShell.Provider.PathNodeProcessors.IProviderContext context) Line 153

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jianyunt Thanks for great clarify! If I understood correctly SHiPS provider does not create a pipeline. I'm still afraid it's a wrong fix - it might break something - we don't have tests to catch this. Perhaps the fix should be in SHiPS provider.

In general 'ProgressRecordType.Completed' status can be important to show a final message to user. With the fix we remove the message. I guess we shouldn't do that.

/cc @daxian-dbw @lzybkr Could you please look the issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe completion, or even the call to PSConsoleHostReadLine (which is how PSReadLine is called from the console host) should set $ProgressPreference = 'SilentlyContinue'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We thought about turning on & off echo for Write-Progress. @BrucePay could you please remind me the story about it?
Fetching data from Azure takes time, so we need to show the progress.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@daxian-dbw I remember you and I had a significant discussion when we were trying to improve the performance in this place - there was a lot of non-obvious things. So I try to be very careful here. :-)
@jianyunt Thanks for great discussion!
Seems we get understanding where right place for the fix (link above) is. I haven't possibility to debug on Unix - if you would grab this it would be great. Otherwise I could do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iSazonov, please go ahead for the fix as I am tied up with other stuff now. Thank you again for the thorough code review.

Copy link
Collaborator

@iSazonov iSazonov Jun 22, 2018

Choose a reason for hiding this comment

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

I can not repo this:

When the Azure cmdlet completes, the hide() gets called. PowerShell handles it properly. But when the WriteProgress for ProgressRecordType.Completed gets called from the SHiPS provider, Show() gets called again. At that time, record.RecordType is ProgressRecordType.Completed.

with script:

0..1 | % {Write-Progress -Id 0 -Activity 11 -PercentComplete $_;
          Sleep 1;
          0..1 | % { Write-Progress -Activity 22 -Id 1 -PercentComplete $_; Sleep 1 };
          Write-Progress -Activity 22 -Id 1 -Completed };
Write-Progress -Activity 11 -Id 0 -Completed

Could you please make a test script to repo the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. won't repro. As @BrucePay also mentioned above, this code path doesn't get hit in the cmdlet scenario. It will repro with APIs.

Copy link
Contributor Author

@jianyunt jianyunt Jun 22, 2018

Choose a reason for hiding this comment

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

You may write an app in C#, call WriteProgress(), then call the above PowerShell script from C#.

Or you may try the following:
On your Linux, install-module azurerm.netcore
install-module azurepsdrive
Login-AzureRmAccount
new-psdrive -name az -psprovider SHiPS -root 'Azurepsdrive#Azure'
cd az:
Get- then tab
you will see the problem
cd az:\AutomationTeam\WebApps
Get- tab, you will see the nested progress issue.

@daxian-dbw daxian-dbw self-assigned this Jun 13, 2018
@daxian-dbw daxian-dbw merged commit 8805ca2 into PowerShell:master Jun 15, 2018
daxian-dbw added a commit that referenced this pull request Jun 19, 2018
daxian-dbw added a commit that referenced this pull request Jun 20, 2018
…b key" (#7117)

Reverts #7023 due to a regression introduced by it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants