Refactor ToolTask output synchronization to use System.Threading.Channels
- Dominant language
- C#
- Stars
- 5.5k
- Forks
- 1.5k
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 141
Description
### Summary
Refactor ToolTask to use System.Threading.Channels for output data synchronization instead of lock/queue + ManualResetEvent pattern.
### Background and Motivation
Currently, ToolTask uses a combination of Queue, ManualResetEvent, and locks to synchronize stdout and stderr output from child processes. This approach is complex and potentially error-prone. In PR #13351, a suggestion was made (and agreed to by @baronfel) to modernize this by using System.Threading.Channels, which would provide a cleaner and more robust producer-consumer abstraction.
The ManualResetEvent + Queue approach is especially challenging to reason about due to race conditions around signaling and resetting, and doesn't mesh well with modern async code. Channel would encapsulate most of the concurrency safety and make the code easier to maintain.
### Proposed Feature
Refactor all relevant data flows in ToolTask (and any helper methods) that rely on enqueuing output lines and signaling availability to instead use Channel (or other appropriate channel type). Have the output-producing callbacks write directly to the channel, and update the consumer (HandleToolNotifications and downstream code) to pull from the channel (potentially asynchronously; see discussion below).
Preserve the priority ordering of tool notifications (timeout, cancel, error, output, exit) from the current WaitHandle.WaitAny pattern, but seek to simplify signaling and logistics by using channels where possible. If introducing async becomes problematic for the sync contract of ToolTask, investigate ways to integrate Channels with WaitHandle or use custom adapters.
Ensure all current regression tests around process output, timeouts, and hangs (e.g. #2981, #10378) are still passing. Consider breaking the refactor into multiple steps if necessary.
### Alternative Designs
- Use Channel.Writer for output callbacks, and Channel.Reader for consumer loop in ToolTask.
- If full async refactoring turns out too invasive, consider a hybrid model with channels and legacy signaling.
- File history: see PR #13351 (https://github.com/dotnet/msbuild/pull/13351) for rationale and previous approaches.
- Related tech debt: There are other places in MSBuild codebase that use lock/queue + ManualResetEvent or similar; consider extracting common patterns or following up if this proves successful.
- This action is supported by recent agreement in the PR review: https://github.com/dotnet/msbuild/pull/13351#discussion_r2976044549
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.