Suppress ExecutionContext flow in BeginErrorReadLine#1498
Conversation
Process.BeginErrorReadLine() captures the caller's ExecutionContext, causing AsyncLocal values to leak to the background stderr reader thread. This is undesirable—the stderr handler is a long-lived background I/O callback that shouldn't inherit ambient state from whichever call site happened to create the transport. Wrap BeginErrorReadLine() in ExecutionContext.SuppressFlow() so the stderr reader gets a clean context. Add a test that sets an AsyncLocal before creating the transport and verifies the stderr callback does NOT see the value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
This is reasonable hygiene, but I'm curious if this is solving some problem you found in the tests? |
|
It was a theory explored as part of the flaky test investigation but it didn't change anything with the test results so I spun it off as a different fix. |
halter73
left a comment
halter73
left a comment
There was a problem hiding this comment.
Without a concrete need, I'm going to close this in order keep the code simple. Let me know if I'm making a mistake here. You know how to reach me.
| // Suppress ExecutionContext flow so the Process's internal async | ||
| // stderr reader thread doesn't capture the caller's ambient context | ||
| // (e.g. AsyncLocal values from test infrastructure or HTTP request state). | ||
| using (ExecutionContext.SuppressFlow()) |
There was a problem hiding this comment.
This is undesirable—the stderr handler is a long-lived background I/O callback that shouldn't inherit ambient state from whichever call site happened to create the transport.
While I kind of agree with this, I also don't want to be muddying every bit of code that kicks off a potentially longish running task with ExecutionContext.SuppressFlow(). And if we do, I would like some clear cut guidance on what counts as a long running thread that shouldn't be allow to hold on to the execution context.
4 participants
Process.BeginErrorReadLine() captures the caller's ExecutionContext, causing AsyncLocal values to leak to the background stderr reader thread. This is undesirable—the stderr handler is a long-lived background I/O callback that shouldn't inherit ambient state from whichever call site happened to create the transport.
Wrap BeginErrorReadLine() in ExecutionContext.SuppressFlow() so the stderr reader gets a clean context.
Add a test that sets an AsyncLocal before creating the transport and verifies the stderr callback does NOT see the value.