Skip to content

Make Out-Default -Transcript more robust in how it handle TranscribeOnly state #3436

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

Merged
lzybkr merged 4 commits into from
Apr 1, 2017
Merged

Make Out-Default -Transcript more robust in how it handle TranscribeOnly state #3436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
58a7996
Modification to TranscribeOnly behavior
PetSerAl Mar 26, 2017
fd76724
Use Interlocked for thread-safety with finalizer thread
PetSerAl Mar 26, 2017
6799cac
Add tests from #3392
SteveL-MSFT Mar 27, 2017
ed6dc29
Rename fields to follow convention
PetSerAl Mar 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions src/System.Management.Automation/commands/utility/FormatAndOutput/out-console/OutConsole.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,9 @@ protected override void BeginProcessing()
mrt.MergeUnclaimedPreviousErrorResults = true;
}

_savedTranscribeOnly = Host.UI.TranscribeOnly;
if (Transcript)
{
Host.UI.TranscribeOnly = true;
_transcribeOnlyCookie = Host.UI.SetTranscribeOnly();
}

// This needs to be done directly through the command runtime, as Out-Default
Expand Down Expand Up @@ -143,15 +142,29 @@ protected override void EndProcessing()
}

base.EndProcessing();
}

if (Transcript)
/// <summary>
/// Revert transcription state on Dispose
/// </summary>
protected override void InternalDispose()
{
try
{
Host.UI.TranscribeOnly = _savedTranscribeOnly;
base.InternalDispose();
}
finally
{
if (_transcribeOnlyCookie != null)
{
_transcribeOnlyCookie.Dispose();
_transcribeOnlyCookie = null;
}
}
}

private ArrayList _outVarResults = null;
private bool _savedTranscribeOnly = false;
private IDisposable _transcribeOnlyCookie = null;
}

/// <summary>
Expand Down
25 changes: 24 additions & 1 deletion src/System.Management.Automation/engine/hostifaces/MshHostUserInterface.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using System.Globalization;
using System.Management.Automation.Runspaces;
using Microsoft.PowerShell.Commands;
using System.Threading;
using System.Threading.Tasks;

namespace System.Management.Automation.Host
Expand Down Expand Up @@ -390,7 +391,29 @@ internal void IgnoreCommand(string commandText, InvocationInfo invocation)
/// so that when content is sent through Out-Default it doesn't
/// make it to the actual host.
/// </summary>
internal bool TranscribeOnly { get; set; }
internal bool TranscribeOnly => Interlocked.CompareExchange(ref _transcribeOnlyCount, 0, 0) != 0;
Copy link
Contributor

@lzybkr lzybkr Mar 28, 2017

Choose a reason for hiding this comment

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

This is pretty confusing for a getter. A getter shouldn't normally change the value (which this doesn't, but when I read CompareExchange, I assume it does and need to read the code more closely).

If you're worried about atomic reads - that's a non-issue, integers are always read atomically on any platform we'll target.

If you're worried about needing a memory barrier, I think you need volatile on the field and use Thread.VolatileRead.

Copy link
Contributor Author

@PetSerAl PetSerAl Mar 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I was worried about stale read, so I wanted to read fresh value of variable. But given that stale read can only occur if destructor kick in other thread, which is already nondeterministic, it should be fine with plain read or volatile one.

private int _transcribeOnlyCount = 0;
internal IDisposable SetTranscribeOnly() => new TranscribeOnlyCookie(this);
private sealed class TranscribeOnlyCookie : IDisposable
{
private PSHostUserInterface _ui;
private bool _disposed = false;
public TranscribeOnlyCookie(PSHostUserInterface ui)
{
_ui=ui;
Interlocked.Increment(ref _ui._transcribeOnlyCount);
}
public void Dispose()
{
if (!_disposed)
{
Interlocked.Decrement(ref _ui._transcribeOnlyCount);
_disposed = true;
GC.SuppressFinalize(this);
}
}
~TranscribeOnlyCookie() => Dispose();
Copy link
Contributor

@lzybkr lzybkr Mar 28, 2017

Choose a reason for hiding this comment

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

I'm not sure we want a destructor.

They are never deterministic - and I think the semantics of transcription should be deterministic.

Copy link
Contributor Author

@PetSerAl PetSerAl Mar 29, 2017

Choose a reason for hiding this comment

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

I do not like to use destructor either, but the choice is to deterministic lock in TranscribeOnly mode or use destructor as safe net.

}

/// <summary>
/// Flag to determine whether the host is transcribing.
Expand Down
40 changes: 40 additions & 0 deletions test/powershell/Modules/Microsoft.PowerShell.Core/Out-Default.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
Describe "Out-Default Tests" -tag CI {
BeforeAll {
# due to https://github.com/PowerShell/PowerShell/issues/3405, `Out-Default -Transcript` emits output to pipeline
# as running in Pester effectively wraps everything in parenthesis, workaround is to use another powershell
# to run the test script passed as a string
$powershell = "$PSHOME/powershell"
}

It "'Out-Default -Transcript' shows up in transcript, but not host" {
$script = @"
`$null = Start-Transcript -Path "$testdrive\transcript.txt";
'hello' | Microsoft.PowerShell.Core\Out-Default -Transcript;
'bye';
`$null = Stop-Transcript
"@

& $powershell -c $script | Should BeExactly 'bye'
"TestDrive:\transcript.txt" | Should Contain 'hello'
}

It "Out-Default reverts transcription state when used more than once in a pipeline" {
& $powershell -c "Out-Default -Transcript | Out-Default -Transcript; 'Hello'" | Should BeExactly "Hello"
}

It "Out-Default reverts transcription state when exception occurs in pipeline" {
& $powershell -c "try { & { throw } | Out-Default -Transcript } catch {}; 'Hello'" | Should BeExactly "Hello"
}

It "Out-Default reverts transcription state even if Dispose() isn't called" {
Copy link
Contributor

@lzybkr lzybkr Mar 28, 2017

Choose a reason for hiding this comment

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

Isn't this just a programmer error to not Dispose?

I don't think we're wrapping a managed resource, and the fact that you need to call WaitForPendingFinalizers to have a reliable test suggests to me that we don't really want this behavior.

Copy link
Contributor Author

@PetSerAl PetSerAl Mar 29, 2017

Choose a reason for hiding this comment

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

Isn't this just a programmer error to not Dispose?

But, how to reliable call Dispose from PowerShell? Assume I wrap Out-Default in SteppablePipeline to make a proxy command with customized behavior:

New-Item Function:Out-Default -Value ([System.Management.Automation.ProxyCommand]::Create((Get-Command Out-Default)))

Then someone throw exception in pipeline with Out-Default -Transcript:

& {throw} | Out-Default -Transcript

And Dispose of proxy command is not called, because PowerShell functions does not have a Dispose. As result Dispose of SteppablePipeline is not called, which lead to Dispose of wrapped Out-Default not called as well.

Copy link
Contributor

@lzybkr lzybkr Mar 29, 2017

Choose a reason for hiding this comment

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

try { <#...#> } finally { $obj.Dispose() } is reliable in PowerShell, the Dispose call will happen even if you hit Ctrl+c to cancel the script.

Copy link
Contributor Author

@PetSerAl PetSerAl Mar 29, 2017

Choose a reason for hiding this comment

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

try/finally can not span thru begin/process/end blocks.

Copy link
Contributor

@lzybkr lzybkr Mar 29, 2017

Choose a reason for hiding this comment

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

No, but a finally block is guaranteed to be called, so it could go in the end block with an empty try.

Copy link
Contributor Author

@PetSerAl PetSerAl Mar 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

But end block is not called as well. If exception occur before your command position in pipeline, then no block of your command is currently in call stack, thus no your try/finally blocks are in effect when exception occur.

Copy link
Contributor

@lzybkr lzybkr Mar 29, 2017

Choose a reason for hiding this comment

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

Yeah, I guess that's the real problem - we don't expose the StopProcessing method in PSCmdlet so there is no way to reliable way to release resources in script.

If that was possible, then how do you feel about adding a destructor?

Copy link
Contributor Author

@PetSerAl PetSerAl Mar 29, 2017

Choose a reason for hiding this comment

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

Then it would be just a programmer error to not Dispose.

$script = @"
`$sp = {Out-Default -Transcript}.GetSteppablePipeline();
`$sp.Begin(`$false);
`$sp = `$null;
[GC]::Collect();
[GC]::WaitForPendingFinalizers();
'hello'
"@
& $powershell -c $script | Should BeExactly 'hello'
}
}