Enable transcription of native commands on non-Windows by SteveL-MSFT · Pull Request #4871 · PowerShell/PowerShell

SteveL-MSFT · 2017-09-20T04:26:52Z

Transcription was relying on reading the screen buffer to record output from native commands.
This resulted in an unhandled exception calling an unimplemented api on non-Windows.
Fix is to default to using redirected output if reading the screen buffer is not supported.

Fix #1920

anmenaga · 2017-09-20T17:53:13Z

test/powershell/Modules/Microsoft.Powershell.Host/Start-Transcript.Tests.ps1

Also need to check that PS error stream is empty.

Actually, there's no reason to follow that pattern, I'm rewriting it as a scriptblock and any errors should just show up in the test run.

anmenaga · 2017-09-20T17:53:28Z

test/powershell/Modules/Microsoft.Powershell.Host/Start-Transcript.Tests.ps1

Need newline in the end of file.

daxian-dbw · 2017-09-20T21:31:40Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

This can be made a static check so that we do it only once. And _scrapeHostOutput can be renamed to s_supportScreenScrape (or something similar).

will change

daxian-dbw · 2017-09-20T21:33:11Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

Why do we want redirectInput to be true by default? Even if we are transcribing and screen scrape is not supported, we shouldn't need to always redirect the input.

And form line 442 below, it indicates that we transcribe for native command only if _runStandAlone is true. Now it looks like you are assuming we can do transcribe regardless of _runStandAlone. That seems not right.

Yes, redirectInput shouldn't be true by default. Will fix.

As for _runStandAlone, my understanding of the code is that if not running standalone, output is redirected and will be captured in the transcript. However, if running standalone, the output is screen scraped and put into the transcript. So it seems that regardless if running standalone, if screen scraping is not supported, the only way to transcribe the output is to redirect it.

daxian-dbw · 2017-09-20T21:45:14Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

Suggestion: this.Command.Context.EngineHostInterface.UI.IsTranscribing is used at 3 places in this class. Maybe we can have an instance field to cache the value? Like _isTranscribing = this.Command.Context.EngineHostInterface.UI.IsTranscribing in the constructor?

daxian-dbw · 2017-09-20T21:53:43Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

from line 444 below, it looks like a HostException also indicates The host doesn't support scraping via its RawUI interface ...

I think the catch for HostException was a mistake. Going through the code in the debugger, that exception isn't thrown.

daxian-dbw · 2017-09-20T22:01:54Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

By putting the call to GetBufferContents in the constructor, it's possible that an arbitrary exception will be thrown from GetBufferContents in the constructor. When that happens, it would be wrapped to an ApplicationFailedException properly prior to this change, but after this change, the NativeCommandProcessor would just fail to be constructed, and thus I guess the error would look obscure too. So, I'm not sure if it's appropriate to put this in the constructor ...

daxian-dbw · 2017-09-20T22:06:41Z

Fix is to default to using redirected output if reading the screen buffer is not supported.

@SteveL-MSFT Could you please explain a little why redirecting the output/error would support transcribing in case that screen scraping is not supported? (just curious how it's done in that case)

SteveL-MSFT · 2017-09-21T00:13:21Z

@daxian-dbw from what I can tell in the code, anything that gets redirected gets transcribed anyways

daxian-dbw · 2017-09-25T20:59:23Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

I think setting redirections for transcribing purpose should be done in the method CalculateIORedirection, and IMO it should be done at the end of that method -- after the existing logic of that method has run. For example, in case of running on the server side, redirectInput should always set to be true. Code like that should run even though screen scraping is not supported.

Makes sense, will fix.

daxian-dbw · 2017-09-25T21:08:32Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

I think there is a change of behavior here. Assuming _isTranscribing == $true and screen scraping is supported,

before the change, _scrapeHostOutput is true only if _runStandAlone is true, so this block will run only the native command is running standalone.

after the change, even if it's not running standalone (i.e. standard input/error redirected), this block of code still will run when transcribing, and in such case, _startPosition is not even initialized.

daxian-dbw · 2017-09-25T22:21:25Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

Maybe remove the else? The else if (Platform.IsWindowsDesktop && IsConsoleApplication(this.Path)) block doesn't necessarily set redirectOutput and redirectError to true.

And please add a comment that explains why we need to set output/error redirection in such case.

daxian-dbw · 2017-09-25T22:23:20Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

These changes now can be reverted.

Will revert

daxian-dbw · 2017-09-25T23:25:45Z

@anmenaga Can you please take a look again?

SteveL-MSFT · 2017-09-26T19:45:57Z

@markekraus the test failures are indicating the WebListener didn't start within the timeout. Is this something you're familiar with?

markekraus · 2017-09-26T19:54:07Z

@SteveL-MSFT News to me. I will take a look.

markekraus · 2017-09-26T20:46:30Z

@SteveL-MSFT This appears to be related to the change in this PR. It looks native execution inside a job is broken. WebListener runs dotnet WebListener.dll in a Start-Job block. From your PR I can manually run the WebListener outside of a job in PowerShell. But inside a job it fails. But it's not limited to WebListener:

$Job = Start-Job {cmd.exe /c echo "test" }
$Job | Wait-Job | fl * -force

$Job.ChildJobs[0].Output

expected behavior (working in beta.7):

State         : Completed
HasMoreData   : True
StatusMessage :
Location      : localhost
Command       : cmd.exe /c echo "test"
JobStateInfo  : Completed
Finished      : System.Threading.ManualResetEvent
InstanceId    : 7d654dd9-ba42-48f0-be72-9e91863e7286
Id            : 1
Name          : Job1
ChildJobs     : {Job2}
PSBeginTime   : 9/26/2017 3:41:02 PM
PSEndTime     : 9/26/2017 3:41:02 PM
PSJobTypeName : BackgroundJob
Output        : {}
Error         : {}
Progress      : {}
Verbose       : {}
Debug         : {}
Warning       : {}
Information   : {}

test

Actual behavior:

State         : Failed
HasMoreData   : False
StatusMessage :
Location      : localhost
Command       : cmd.exe /c echo "test"
JobStateInfo  : Failed
Finished      : System.Threading.ManualResetEvent
InstanceId    : 58c30632-c4a1-41e3-b276-5aa9b24db3b0
Id            : 3
Name          : Job3
ChildJobs     : {Job4}
PSBeginTime   : 9/26/2017 3:42:41 PM
PSEndTime     : 9/26/2017 3:42:42 PM
PSJobTypeName : BackgroundJob
Output        : {}
Error         : {}
Progress      : {}
Verbose       : {}
Debug         : {}
Warning       : {}
Information   : {}

SteveL-MSFT · 2017-09-26T20:50:16Z

@markekraus thanks! I'll take a look.

The problem is that when run as a job obviously there's no console, so some of the console calls even on Windows throws other exceptions (and it's not just the Host.HostException from the old code) when the code is checking if screen scraping is supported. Added a test to test start-job with native command to cover this.

@daxian-dbw @anmenaga can you take another look?

…ot supported since screen buffer reading isn't supported, default to using redirected output

moved setting redirection due to transcription to within CalculateIORedirection() function ensure screen scrapping only happens when running standalone

address PR feedback from Dongbo

added test for start-job and native commands catch all exceptions trying to read the buffer

daxian-dbw · 2017-09-27T20:16:18Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+            catch (Exception)
+            {
+                // screen scraping not supported on non-Windows or as job
+            }


I think this check can still be done only once -- we do this check in CalculateIORedirection and only if _runStandAlone is true. Here the the code I propose:

private void CalculateIORedirection(out bool redirectOutput, out bool redirectError, out bool redirectInput) { ... if (NativeCommandProcessor.IsServerSide) { redirectInput = true; redirectOutput = true; redirectError = true; } else if (Platform.IsWindowsDesktop && IsConsoleApplication(this.Path)) { // On Windows desktops, if the command to run is a console application, // then allocate a console if there isn't one attached already... ConsoleVisibility.AllocateHiddenConsole(); if (ConsoleVisibility.AlwaysCaptureApplicationIO) { redirectOutput = true; redirectError = true; } } _runStandAlone = !redirectInput && !redirectOutput && !redirectError; // !!!!! NOTE !!!!! -- Unchanged until here if (_runStandAlone) { if (s_supportScreenScrape == null) { // Call GetBufferContents() to check and set s_supportScreenScrape to true or false accordingly. } // if screen scraping isn't supported, we enable redirection so that the output is still transcribed // as redirected output is always transcribed if (_isTranscribing && (false == s_supportScreenScrape)) { redirectOutput = true; redirectError = true; _runStandAlone = false; } } }

So the check of screen scraping support is done only if we are running the native command as standalone application.

In case of running it in a Job, I think the standard input and error are already redirected, so nothing needs to be done.

I see. I'll try that.

refactor code

daxian-dbw

LGTM. Thanks!

SteveL-MSFT assigned daxian-dbw Sep 20, 2017

SteveL-MSFT requested a review from anmenaga September 20, 2017 04:26

SteveL-MSFT requested review from BrucePay, daxian-dbw, lzybkr and vors as code owners September 20, 2017 04:26

msftclas added the cla-not-required label Sep 20, 2017

anmenaga suggested changes Sep 20, 2017

View reviewed changes

daxian-dbw requested changes Sep 20, 2017

View reviewed changes

SteveL-MSFT requested review from HemantMahawar, JamesWTruher, PaulHigin, TravisEz13, adityapatwardhan, dantraMSFT and joeyaiello as code owners September 23, 2017 00:06

SteveL-MSFT force-pushed the mac-transcript branch from 722b161 to c72288d Compare September 23, 2017 00:07

vors removed their request for review September 25, 2017 00:39

daxian-dbw requested changes Sep 25, 2017

View reviewed changes

daxian-dbw approved these changes Sep 25, 2017

View reviewed changes

SteveL-MSFT force-pushed the mac-transcript branch from 015ca77 to f769df1 Compare September 25, 2017 23:58

msftgits removed the cla-not-required label Sep 26, 2017

SteveL-MSFT force-pushed the mac-transcript branch from f769df1 to b63b374 Compare September 26, 2017 18:54

anmenaga approved these changes Sep 26, 2017

View reviewed changes

SteveL-MSFT added 6 commits September 26, 2017 15:36

handle exception for non-Windows where reading the screen buffer is n…

f39b1d6

…ot supported since screen buffer reading isn't supported, default to using redirected output

address PR feedback

5ad677f

address Dongbo's PR feedback

7373ea5

[feature]

c68cb39

moved setting redirection due to transcription to within CalculateIORedirection() function ensure screen scrapping only happens when running standalone

[feature]

0f2b52a

address PR feedback from Dongbo

[feature]

2ff64d8

added test for start-job and native commands catch all exceptions trying to read the buffer

SteveL-MSFT force-pushed the mac-transcript branch from b63b374 to 2ff64d8 Compare September 26, 2017 22:50

daxian-dbw requested changes Sep 27, 2017

View reviewed changes

[feature]

bb5767d

refactor code

daxian-dbw approved these changes Sep 28, 2017

View reviewed changes

daxian-dbw merged commit 99e3fe5 into PowerShell:master Sep 29, 2017

SteveL-MSFT deleted the mac-transcript branch October 26, 2018 21:33

Conversation

SteveL-MSFT commented Sep 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Sep 20, 2017

Uh oh!

SteveL-MSFT commented Sep 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Sep 25, 2017

Uh oh!

SteveL-MSFT commented Sep 26, 2017

Uh oh!

markekraus commented Sep 26, 2017

Uh oh!

markekraus commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveL-MSFT commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daxian-dbw Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Sep 20, 2017 •

edited

Loading

markekraus commented Sep 26, 2017 •

edited

Loading

SteveL-MSFT commented Sep 26, 2017 •

edited

Loading

daxian-dbw Sep 27, 2017 •

edited

Loading