Make `Out-Default -Transcript` more robust in how it handle `TranscribeOnly` state by PetSerAl · Pull Request #3436 · PowerShell/PowerShell

PetSerAl · Mar 27, 2017

Addresses #3390

msftclas · Mar 27, 2017

@PetSerAl,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

SteveL-MSFT · Mar 27, 2017


        private ArrayList _outVarResults = null;
-        private bool _savedTranscribeOnly = false;
+        private IDisposable transcribeOnlyCookie = null;


should follow the convention in this file and call it _transcribeOnlyCookie

SteveL-MSFT · Mar 27, 2017

        /// </summary>
-        internal bool TranscribeOnly { get; set; }
+        internal bool TranscribeOnly => Interlocked.CompareExchange(ref transcribeOnlyCount, 0, 0) != 0;
+        private int transcribeOnlyCount = 0;


_transcribeOnlyCount to follow existing convention

SteveL-MSFT · Mar 27, 2017

+        private sealed class TranscribeOnlyCookie : IDisposable
+        {
+            private PSHostUserInterface ui;
+            private bool disposed = false;


_disposed to follow existing convention

SteveL-MSFT · Mar 27, 2017

+        internal IDisposable SetTranscribeOnly() => new TranscribeOnlyCookie(this);
+        private sealed class TranscribeOnlyCookie : IDisposable
+        {
+            private PSHostUserInterface ui;


_ui to follow existing convention

lzybkr · Mar 28, 2017

        /// make it to the actual host.
        /// </summary>
-        internal bool TranscribeOnly { get; set; }
+        internal bool TranscribeOnly => Interlocked.CompareExchange(ref _transcribeOnlyCount, 0, 0) != 0;


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.

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.

lzybkr · Mar 28, 2017

+                    GC.SuppressFinalize(this);
+                }
+            }
+            ~TranscribeOnlyCookie() => Dispose();


I'm not sure we want a destructor.

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

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

lzybkr · Mar 28, 2017

+        & $powershell -c "try { & { throw } | Out-Default -Transcript } catch {}; 'Hello'" | Should BeExactly "Hello"
+    }
+
+    It "Out-Default reverts transcription state even if Dispose() isn't called" {


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.

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.

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

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

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

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.

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?

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

PetSerAl · Apr 5, 2017

@lzybkr Should associated issue (#3390) be closed as well?

lzybkr · Apr 5, 2017

Indeed, I forget to do that pretty much every time I merge, you'd think I could remember by now.

PetSerAl and others added 3 commits March 26, 2017 14:22

Modification to TranscribeOnly behavior

58a7996

Use Interlocked for thread-safety with finalizer thread

fd76724

Add tests from #3392

6799cac

msftclas added the cla-already-signed label Mar 27, 2017

PetSerAl changed the title ~~Make Out-Default -Transcript more robust in how it handle TranscriptOnly state~~ Make Out-Default -Transcript more robust in how it handle TranscribeOnly state Mar 27, 2017

SteveL-MSFT requested a review from lzybkr March 27, 2017 23:46

SteveL-MSFT requested changes Mar 27, 2017

View reviewed changes

Rename fields to follow convention

ed6dc29

SteveL-MSFT approved these changes Mar 28, 2017

View reviewed changes

daxian-dbw assigned lzybkr Mar 28, 2017

lzybkr reviewed Mar 28, 2017

View reviewed changes

lzybkr merged commit f76b2fc into PowerShell:master Apr 1, 2017

PetSerAl deleted the transcribeonly-patch branch October 1, 2017 06:17

This was referenced May 12, 2023

Write-Output broken PowerShell/vscode-powershell#3991

Closed

Fix disappearing output in PowerShell 5.1 PowerShell/PowerShellEditorServices#2023

Merged

Search code, repositories, users, issues, pull requests...

Conversation

PetSerAl commented Mar 27, 2017

Uh oh!

msftclas commented Mar 27, 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!

PetSerAl Mar 29, 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!

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!

PetSerAl Mar 29, 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!

Choose a reason for hiding this comment

Uh oh!

PetSerAl commented Apr 5, 2017

Uh oh!

lzybkr commented Apr 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PetSerAl Mar 29, 2017 •

edited

Loading

PetSerAl Mar 29, 2017 •

edited

Loading