Make Out-Default -Transcript more robust in how it handle TranscribeOnly state
#3436
Conversation
|
@PetSerAl, |
PetSerAl
changed the title
Out-Default -Transcript more robust in how it handle TranscriptOnly stateOut-Default -Transcript more robust in how it handle TranscribeOnly state
|
|
||
| private ArrayList _outVarResults = null; | ||
| private bool _savedTranscribeOnly = false; | ||
| private IDisposable transcribeOnlyCookie = null; |
There was a problem hiding this comment.
should follow the convention in this file and call it _transcribeOnlyCookie
| /// </summary> | ||
| internal bool TranscribeOnly { get; set; } | ||
| internal bool TranscribeOnly => Interlocked.CompareExchange(ref transcribeOnlyCount, 0, 0) != 0; | ||
| private int transcribeOnlyCount = 0; |
There was a problem hiding this comment.
_transcribeOnlyCount to follow existing convention
| private sealed class TranscribeOnlyCookie : IDisposable | ||
| { | ||
| private PSHostUserInterface ui; | ||
| private bool disposed = false; |
There was a problem hiding this comment.
_disposed to follow existing convention
| internal IDisposable SetTranscribeOnly() => new TranscribeOnlyCookie(this); | ||
| private sealed class TranscribeOnlyCookie : IDisposable | ||
| { | ||
| private PSHostUserInterface ui; |
There was a problem hiding this comment.
_ui to follow existing convention
| /// make it to the actual host. | ||
| /// </summary> | ||
| internal bool TranscribeOnly { get; set; } | ||
| internal bool TranscribeOnly => Interlocked.CompareExchange(ref _transcribeOnlyCount, 0, 0) != 0; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| GC.SuppressFinalize(this); | ||
| } | ||
| } | ||
| ~TranscribeOnlyCookie() => Dispose(); |
There was a problem hiding this comment.
I'm not sure we want a destructor.
They are never deterministic - and I think the semantics of transcription should be deterministic.
There was a problem hiding this comment.
I do not like to use destructor either, but the choice is to deterministic lock in TranscribeOnly mode or use destructor as safe net.
| & $powershell -c "try { & { throw } | Out-Default -Transcript } catch {}; 'Hello'" | Should BeExactly "Hello" | ||
| } | ||
|
|
||
| It "Out-Default reverts transcription state even if Dispose() isn't called" { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 -TranscriptAnd 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.
There was a problem hiding this comment.
try { <#...#> } finally { $obj.Dispose() } is reliable in PowerShell, the Dispose call will happen even if you hit Ctrl+c to cancel the script.
There was a problem hiding this comment.
try/finally can not span thru begin/process/end blocks.
There was a problem hiding this comment.
No, but a finally block is guaranteed to be called, so it could go in the end block with an empty try.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Then it would be just a programmer error to not Dispose.
|
Indeed, I forget to do that pretty much every time I merge, you'd think I could remember by now. |
4 participants
Addresses #3390