`Process.OutputReadNotifyUser` does not respect the synchronization context when invoking `DataReceivedEventHandler`

[daxian-dbw]

Summary

This was reported in PowerShell and turned out to be a .NET Core issue: PowerShell/PowerShell#11658

In .NET Framework, Process.OutputReadNotifyUser respects the synchronization context and depends on SynchronizingObject to invoke the handler on the desired thread.

However, in .NET Core 3.1, Process.OutputReadNotifyUser doesn't respect the SynchronizingObject anymore. The callback will eventually be invoked in AsyncStreamReader.ReadBufferAsync, after await _stream.ReadAsync(...).ConfigureAwait(false). Be noted about the ConfigureAwait(false) part, it means everything after this will be continued on a thread pool thread, and thus the DataReceivedEventHandler will always be invoked on a thread pool thread instead.

Reproduce steps

I don't have a simpler repro handy, and below is the original repro from the PowerShell issue.
Paste below code into Windows PowerShell 5.1 and pwsh 7.0.0-rc.2, then click Run button on the form window.

[reflection.assembly]::LoadWithPartialName("System.Windows.Forms") | Out-Null
[reflection.assembly]::LoadWithPartialName("System.Drawing") | Out-Null

[System.Windows.Forms.Application]::EnableVisualStyles()
$form = New-Object 'System.Windows.Forms.Form'
$buttonRunProcess = New-Object 'System.Windows.Forms.Button'
$richtextboxOutput = New-Object 'System.Windows.Forms.RichTextBox'
$buttonExit = New-Object 'System.Windows.Forms.Button'
$InitialFormWindowState = New-Object 'System.Windows.Forms.FormWindowState'

$buttonExit_Click={
    $form.Close()
}

$buttonRunProcess_Click = {
    $buttonRunProcess.Enabled = $false

    $richtextboxOutput.Clear()

    $process = New-Object System.Diagnostics.Process
    $process.StartInfo.FileName = 'ping.exe'
    $process.StartInfo.Arguments = 'google.com'
    $process.StartInfo.UseShellExecute = $false
    $process.StartInfo.CreateNoWindow = $true
    $process.StartInfo.RedirectStandardInput = $false
    $process.StartInfo.RedirectStandardOutput = $true
    $process.EnableRaisingEvents = $true
    $process.SynchronizingObject = $buttonRunProcess
    $process.add_OutputDataReceived( {
        # Use $_.Data to access the output text
        $richtextboxOutput.AppendText($_.Data)
        $richtextboxOutput.AppendText("`r`n") }
)

    $process.Start() | Out-Null
    $process.BeginOutputReadLine()
}


$Form_StateCorrection_Load=
{
    #Correct the initial state of the form to prevent the .Net maximized form issue
    $form.WindowState = $InitialFormWindowState
}

$Form_Cleanup_FormClosed=
{
    try
    {
        $buttonRunProcess.remove_Click($buttonRunProcess_Click)
        $buttonExit.remove_Click($buttonExit_Click)
        $form.remove_FormClosed($processTracker_FormClosed)
        $form.remove_Load($Form_StateCorrection_Load)
        $form.remove_FormClosed($Form_Cleanup_FormClosed)
    }
    catch { Out-Null <# Prevent PSScriptAnalyzer warning #> }
}

$form.SuspendLayout()

$form.Controls.Add($buttonRunProcess)
$form.Controls.Add($richtextboxOutput)
$form.Controls.Add($buttonExit)
$form.ClientSize = [System.Drawing.Size]::new(584, 362)
$form.Margin = '4, 4, 4, 4'
$form.MinimumSize = [System.Drawing.Size]::new(304, 315)
$form.Name = 'Redirect Process Output'
$form.StartPosition = 'CenterScreen'
$form.Text = 'Redirect Process Output'

$buttonRunProcess.Anchor = 'Bottom, Left'
$buttonRunProcess.Location = [System.Drawing.Point]::new(12, 327)
$buttonRunProcess.Name = 'buttonRunProcess'
$buttonRunProcess.Size = [System.Drawing.Size]::new(75, 23)
$buttonRunProcess.TabIndex = 0
$buttonRunProcess.Text = 'Run'
$buttonRunProcess.UseCompatibleTextRendering = $True
$buttonRunProcess.UseVisualStyleBackColor = $True
$buttonRunProcess.add_Click($buttonRunProcess_Click)

$richtextboxOutput.Anchor = 'Top, Bottom, Left, Right'
$richtextboxOutput.HideSelection = $False
$richtextboxOutput.Location = [System.Drawing.Point]::new(12, 12)
$richtextboxOutput.Name = 'richtextboxOutput'
$richtextboxOutput.ReadOnly = $True
$richtextboxOutput.Size = [System.Drawing.Size]::new(559, 305)
$richtextboxOutput.TabIndex = 6
$richtextboxOutput.Text = ''
$richtextboxOutput.WordWrap = $False

$buttonExit.Anchor = 'Bottom, Right'
$buttonExit.Location = [System.Drawing.Point]::new(497, 327)
$buttonExit.Name = 'buttonExit'
$buttonExit.Size =  [System.Drawing.Size]::new(75, 23)
$buttonExit.TabIndex = 2
$buttonExit.Text = 'E&xit'
$buttonExit.UseCompatibleTextRendering = $True
$buttonExit.UseVisualStyleBackColor = $True
$buttonExit.add_Click($buttonExit_Click)

$form.ResumeLayout()

$InitialFormWindowState = $form.WindowState
$form.add_Load($Form_StateCorrection_Load)
$form.add_FormClosed($Form_Cleanup_FormClosed)
$form.ShowDialog()

Expected behavior

After clicking the Run button the output from ping google.com is displayed inside the text box on both Windows PowerShell and pwsh 7.

Actual behavior

It works fine on Windows PowerShell.

However on pwsh 7, no output is displayed and pwsh crashes with the following exception.

Unhandled exception. System.Management.Automation.PSInvalidOperationException: There is no Runspace available to run scripts in this thread. You can provide one in the DefaultRunspace property of the System.Management.Automation.Runspaces.Runspace type. The script block you attempted to invoke was:
        # Use $_.AppendText("`r`n")
   at System.Management.Automation.ScriptBlock.GetContextFromTLS()
   at System.Management.Automation.ScriptBlock.InvokeAsDelegateHelper(Object dollarUnder, Object dollarThis, Object[] args)
   at lambda_method(Closure , Object , DataReceivedEventArgs )
   at System.Diagnostics.Process.OutputReadNotifyUser(String data)
   at System.Diagnostics.AsyncStreamReader.FlushMessageQueue(Boolean rethrowInNewThread)
--- End of stack trace from previous location where exception was thrown ---
   at System.Diagnostics.AsyncStreamReader.<>c.<FlushMessageQueue>b__18_0(Object edi)
   at System.Threading.QueueUserWorkItemCallback.<>c.<.cctor>b__6_0(QueueUserWorkItemCallback quwi)
   at System.Threading.ExecutionContext.RunForThreadPoolUnsafe[TState](ExecutionContext executionContext, Action`1 callback, TState& state)
   at System.Threading.QueueUserWorkItemCallback.Execute()
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading._ThreadPoolWaitCallback.PerformWaitCallback()

This is because the event handler to OutputDataReceived is a delegate created from a PowerShell script block, and it requires a default Runspace from the current thread to execute.
On Windows PowerShell, the handler is invoked on the right thread thanks to the sync context.
On .NET Core 3.1, it's invoked on a thread pool thread, which doesn't have a default Runspace and hence the exception.

$process.add_OutputDataReceived( {
        # Use $_.Data to access the output text
        $richtextboxOutput.AppendText($_.Data)
        $richtextboxOutput.AppendText("`r`n") }
)

Environment data

Windows PowerShell 5.1 runs on .NET Framework, and pwsh 7.0.0-rc.2 runs on .NET Core 3.1

[ALIENQuake]

Hi,

Let me share the bigger context of what is broken by this bug: all admin-gui-like tools (PS7-based) which are produced by SysAdmins/DevOps in order to handle repetitive tasks by employers who aren't proficient with Powershell but the can use such simple tools for simple tasks and debugging purposes.

Example: https://i.imgur.com/zj13XVh.gifv

When I compare the source code of Net and Net Core, it seems to me that it also affects other events like "onExited" etc.

[Halkcyon]

@ALIENQuake Good observation. While tracking down a crashing script I discovered I was trying to hook into a process Exited event and when it fired, the whole app crashes (in this case, PowerShell).

Anyone know if there's a workaround for this? Found this thread from PowerShell/PowerShell#11658

[ericstj]

I believe this is a side-effect of the fact that the SynchronizingObject property was originally omitted from this API. It was later added, but not plumbed through the implementation 2579595.

It's not missing from only OutputDataReceived, but also Exited and ErrorDataReceived.

I believe we need to bring back this pattern on all three events:
https://github.com/microsoft/referencesource/blob/a7bd3242bd7732dec4aebb21fbc0f6de61c2545e/System/services/monitoring/system/diagnosticts/Process.cs#L1636-L1639

I double checked the other cases where we expose an ISynchronizeInvoke and this seems to be the only place we missed it. Let's try and fix that up this release.

[ALIENQuake]

@ericstj @stephentoub Please accept my big thanks, I had a lot of worries about does such little bug and rare use-case would be something that would be worth fixing for the NET team. Thank you very much!