NamedPipeServer callbacks do no async work. so make the signature sync#7507
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the NamedPipeServer request callback contract to be synchronous, removing unnecessary async/Task usage across the IPC stack (platform hosts, MSBuild task, extensions, and unit tests).
Changes:
- Changed
NamedPipeServercallback type fromFunc<IRequest, Task<IResponse>>toFunc<IRequest, IResponse>and removed theawaitat the invocation site. - Updated all in-repo call sites (hosts, MSBuild task, extensions) and unit tests to provide synchronous callbacks returning
IResponsedirectly. - Converted a few callback implementations that only performed async logging to use synchronous logging APIs.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/Microsoft.Testing.Platform.UnitTests/IPC/IPCTests.cs | Updates unit test callbacks to return IResponse synchronously. |
| src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs | Changes the callback delegate type to synchronous and invokes it without await. |
| src/Platform/Microsoft.Testing.Platform/Hosts/TestHostControllersTestHost.cs | Updates the IPC server callback handler to be synchronous. |
| src/Platform/Microsoft.Testing.Platform.MSBuild/Tasks/InvokeTestingPlatformTask.cs | Updates MSBuild IPC server request handler to be synchronous. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxProcessLifetimeHandler.cs | Updates TRX extension IPC callback to be synchronous. |
| src/Platform/Microsoft.Testing.Extensions.Retry/RetryFailedTestsPipeServer.cs | Updates retry extension IPC callback to be synchronous. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/HangDumpProcessLifetimeHandler.cs | Updates hang dump lifetime handler IPC callback to be synchronous and switches debug logging to sync. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/HangDumpActivityIndicator.cs | Updates hang dump activity indicator IPC callback to be synchronous and switches debug logging to sync. |
Comments suppressed due to low confidence (1)
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs:46
- Changing NamedPipeServer's callback type from Func<IRequest, Task> to Func<IRequest, IResponse> is a binary breaking change for friend assemblies (the Platform project exposes internals to multiple Extensions projects). Given this file already keeps compatibility overloads to avoid MissingMethodException when core and extension packages are version-skewed, consider adding compatibility constructor overload(s) that still accept Func<IRequest, Task> and adapt internally, so older extensions can continue to load against a newer core.
public NamedPipeServer(
string name,
Func<IRequest, IResponse> callback,
IEnvironment environment,
ILogger logger,
ITask task,
CancellationToken cancellationToken)
| if (request is GetInProgressTestsRequest) | ||
| { | ||
| await _logger.LogDebugAsync($"Received '{nameof(GetInProgressTestsRequest)}'").ConfigureAwait(false); | ||
| _logger.LogDebug($"Received '{nameof(GetInProgressTestsRequest)}'"); |
There was a problem hiding this comment.
This is potentially running on a threadpool thread, and the logger implementation is very likely synchronized via a semaphore. Waiting on the semaphore synchronously might lead to blocking threadpool thread in case there is some contention in logging, and I think this is a risky change to make.
There was a problem hiding this comment.
does that mean the sync log APIs should be removed? and all callers of them made async?
There was a problem hiding this comment.
I wouldn't say it should be removed, but we could revise usages to switch to async when it makes sense.
For example, if we are running something in a custom non-threadpool thread, the sync version makes sense, and (depending on the exact work done on that thread) might even be preferred over the async variant. But in other cases, the async version will make more sense (especially when it's something already running on a threadpool thread)
No description provided.