Skip to content

Ensure dotnet test messages are received in the right timings/modes #50727

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 10, 2025
Merged

Ensure dotnet test messages are received in the right timings/modes #50727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/Cli/dotnet/Commands/CliCommandStrings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2682,4 +2682,13 @@ Proceed?</value>
<data name="UnexpectedHelpMessage" xml:space="preserve">
<value>An unexpected help-related message was received when `--help` wasn't used.</value>
</data>
</root>
<data name="MismatchingHandshakeInfo" xml:space="preserve">
<value>A new handshake '{0}' was received for the test application that doesn't match the previous handshake '{1}'.</value>
</data>
<data name="UnexpectedMessageInHelpMode" xml:space="preserve">
<value>A message of type '{0}' was received in help mode, which is not expected.</value>
</data>
<data name="UnexpectedMessageWithoutHandshake" xml:space="preserve">
<value>A message of type '{0}' was received before we got any handshakes, which is unexpected.</value>
</data>
</root>
2 changes: 1 addition & 1 deletion src/Cli/dotnet/Commands/Test/MTP/IPC/Models/HandshakeMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@

namespace Microsoft.DotNet.Cli.Commands.Test.IPC.Models;

internal sealed record HandshakeMessage(Dictionary<byte, string>? Properties) : IRequest, IResponse;
internal sealed record HandshakeMessage(Dictionary<byte, string> Properties) : IRequest, IResponse;
2 changes: 0 additions & 2 deletions src/Cli/dotnet/Commands/Test/MTP/IPC/Serializers/HandshakeMessageSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

#nullable disable

using System.Diagnostics;
using Microsoft.DotNet.Cli.Commands.Test.IPC.Models;

Expand Down
42 changes: 20 additions & 22 deletions src/Cli/dotnet/Commands/Test/MTP/TestApplication.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

#nullable disable

using System.Diagnostics;
using System.Globalization;
using System.IO.Pipes;
using System.Threading;
using Microsoft.DotNet.Cli.Commands.Test.IPC;
using Microsoft.DotNet.Cli.Commands.Test.IPC.Models;
using Microsoft.DotNet.Cli.Commands.Test.IPC.Serializers;
Expand All @@ -28,9 +27,7 @@ internal sealed class TestApplication(
private readonly List<string> _outputData = [];
private readonly List<string> _errorData = [];
private readonly string _pipeName = NamedPipeServer.GetPipeName(Guid.NewGuid().ToString("N"));
private readonly CancellationTokenSource _cancellationToken = new();

private Task _testAppPipeConnectionLoop;
private readonly List<NamedPipeServer> _testAppPipeConnections = [];
private readonly Dictionary<NamedPipeServer, HandshakeMessage> _handshakes = new();

Expand All @@ -45,17 +42,26 @@ public async Task<int> RunAsync()
// Consider throwing an exception if it's called more than once.
var processStartInfo = CreateProcessStartInfo();

_testAppPipeConnectionLoop = Task.Run(async () => await WaitConnectionAsync(_cancellationToken.Token), _cancellationToken.Token);
var testProcessExitCode = await StartProcess(processStartInfo);
var cancellationTokenSource = new CancellationTokenSource();
var cancellationToken = cancellationTokenSource.Token;
Task? testAppPipeConnectionLoop = null;

WaitOnTestApplicationPipeConnectionLoop();
try
{
testAppPipeConnectionLoop = Task.Run(async () => await WaitConnectionAsync(cancellationToken), cancellationToken);
var testProcessExitCode = await StartProcess(processStartInfo);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we pass cancellationToken to this? it seems that once we start the process we do not cancel here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. It's very possible that cancellation doesn't work very well today. @mariam-abdulla Do you know please?

Copy link
Member Author

@Youssef1313 Youssef1313 Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's follow-up in a separate issue either way. #50732

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MTP handle the CTRL+C in a graceful way, here if I'm not mistaken we can only kill the process after the start and it's not graceful. So we can add a check on the boolean and don't start the process for "performance optimization" but we should avoid the kill.

Or add a client->server call(we're push only today) to signal the processes if we expect to "logically cancel" out of CTRL+C.

if (_handler.HasMismatchingTestSessionEventCount())
{
throw new InvalidOperationException(CliCommandStrings.MissingTestSessionEnd);
}

if (_handler.HasMismatchingTestSessionEventCount())
return testProcessExitCode;
}
finally
{
throw new InvalidOperationException(CliCommandStrings.MissingTestSessionEnd);
cancellationTokenSource.Cancel();
testAppPipeConnectionLoop?.Wait((int)TimeSpan.FromSeconds(30).TotalMilliseconds);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we await it? the VS client uses the following vs-threading API https://github.com/microsoft/vs-threading/blob/849676611f654d73c84a9468b0ecc205294250e0/src/Microsoft.VisualStudio.Threading/ThreadingTools.cs#L105

also, when would we expect a timeout? if test process exits, then this method should hopefully completely pretty much immediately

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following-up in #50733

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I go agree that it's expected for this task to complete almost immediately.

Copy link
Member

@MarcoRossignoli MarcoRossignoli Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I advice to keep always a timeout to avoid in case of bugs to hang forever and have a clear stack trace issue, when an api accept a time we should always set it.

}

return testProcessExitCode;
}

private ProcessStartInfo CreateProcessStartInfo()
Expand Down Expand Up @@ -145,12 +151,6 @@ private string GetArguments()
return builder.ToString();
}

private void WaitOnTestApplicationPipeConnectionLoop()
{
_cancellationToken.Cancel();
_testAppPipeConnectionLoop?.Wait((int)TimeSpan.FromSeconds(30).TotalMilliseconds);
}

private async Task WaitConnectionAsync(CancellationToken token)
{
try
Expand Down Expand Up @@ -240,7 +240,7 @@ private Task<IResponse> OnRequest(NamedPipeServer server, IRequest request)

private static string GetSupportedProtocolVersion(HandshakeMessage handshakeMessage)
{
if (!handshakeMessage.Properties.TryGetValue(HandshakeMessagePropertyNames.SupportedProtocolVersions, out string protocolVersions) ||
if (!handshakeMessage.Properties.TryGetValue(HandshakeMessagePropertyNames.SupportedProtocolVersions, out string? protocolVersions) ||
protocolVersions is null)
{
// It's not expected we hit this.
Expand All @@ -263,7 +263,7 @@ private static string GetSupportedProtocolVersion(HandshakeMessage handshakeMess
}

private static HandshakeMessage CreateHandshakeMessage(string version) =>
new(new Dictionary<byte, string>(capacity: 5)
new HandshakeMessage(new Dictionary<byte, string>(capacity: 5)
{
{ HandshakeMessagePropertyNames.PID, Environment.ProcessId.ToString(CultureInfo.InvariantCulture) },
{ HandshakeMessagePropertyNames.Architecture, RuntimeInformation.ProcessArchitecture.ToString() },
Expand All @@ -276,7 +276,7 @@ private async Task<int> StartProcess(ProcessStartInfo processStartInfo)
{
Logger.LogTrace($"Test application arguments: {processStartInfo.Arguments}");

using var process = Process.Start(processStartInfo);
using var process = Process.Start(processStartInfo)!;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the maximum parallelism level for the dotnet CLI? have we tested with lots of test projects? we've seen Process.Start taking over the threadpool threads and this was causing contention in the VS client

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We default to Environment.ProcessorCount unless specified by the user.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@drognanar What was the fix for the threadpool starvation issue on VS side? What would be the right way for dotnet test in this case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#50734

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used a combination of calling https://learn.microsoft.com/en-us/dotnet/api/system.threading.threadpool.setminthreads?view=net-9.0 to increase the threadpool size and specifically for the Process.Start it was put under an AsyncLock so that effectively one thread can enter this code path. this is because internally Process.Start runs under a lock on Windows and blocks a threadpool thread from running.

StoreOutputAndErrorData(process);
await process.WaitForExitAsync();

Expand Down Expand Up @@ -398,7 +398,5 @@ public void Dispose()
Reporter.Error.WriteLine(messageBuilder.ToString());
}
}

WaitOnTestApplicationPipeConnectionLoop();
}
}
56 changes: 32 additions & 24 deletions src/Cli/dotnet/Commands/Test/MTP/TestApplicationHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,18 @@ internal void OnHandshakeReceived(HandshakeMessage handshakeMessage, bool gotSup
return;
}

var executionId = handshakeMessage.Properties[HandshakeMessagePropertyNames.ExecutionId];
var arch = handshakeMessage.Properties[HandshakeMessagePropertyNames.Architecture]?.ToLower();
var tfm = TargetFrameworkParser.GetShortTargetFramework(handshakeMessage.Properties[HandshakeMessagePropertyNames.Framework]);
var currentHandshakeInfo = (tfm, arch, executionId);

if (!_handshakeInfo.HasValue)
{
var executionId = handshakeMessage.Properties[HandshakeMessagePropertyNames.ExecutionId];
var arch = handshakeMessage.Properties[HandshakeMessagePropertyNames.Architecture]?.ToLower();
var tfm = TargetFrameworkParser.GetShortTargetFramework(handshakeMessage.Properties[HandshakeMessagePropertyNames.Framework]);

_handshakeInfo = (tfm, arch, executionId);
_handshakeInfo = currentHandshakeInfo;
}
else
else if (_handshakeInfo.Value != currentHandshakeInfo)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

under what circumstances do we expect multiple handshakes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestHostController+TestHost, or an orchestrator (e.g, Retry, or a future sharding orchestrator)

{
// TODO: Verify we get the same info.
throw new InvalidOperationException(string.Format(CliCommandStrings.MismatchingHandshakeInfo, currentHandshakeInfo, _handshakeInfo.Value));
}

var hostType = handshakeMessage.Properties[HandshakeMessagePropertyNames.HostType];
Expand Down Expand Up @@ -91,13 +92,14 @@ internal void OnDiscoveredTestsReceived(DiscoveredTestMessages discoveredTestMes
{
LogDiscoveredTests(discoveredTestMessages);

// TODO: If _handshakeInfo is null, we should error.
// We shouldn't be getting any discovered test messages without a previous handshake.

if (_options.IsHelp)
{
// TODO: Better to throw exception?
return;
throw new InvalidOperationException(string.Format(CliCommandStrings.UnexpectedMessageInHelpMode, nameof(DiscoveredTestMessages)));
}

if (!_handshakeInfo.HasValue)
{
throw new InvalidOperationException(string.Format(CliCommandStrings.UnexpectedMessageWithoutHandshake, nameof(DiscoveredTestMessages)));
}

foreach (var test in discoveredTestMessages.DiscoveredMessages)
Expand All @@ -112,13 +114,14 @@ internal void OnTestResultsReceived(TestResultMessages testResultMessage)
{
LogTestResults(testResultMessage);

// TODO: If _handshakeInfo is null, we should error.
// We shouldn't be getting any test result messages without a previous handshake.

if (_options.IsHelp)
{
// TODO: Better to throw exception?
return;
throw new InvalidOperationException(string.Format(CliCommandStrings.UnexpectedMessageInHelpMode, nameof(TestResultMessages)));
}

if (!_handshakeInfo.HasValue)
{
throw new InvalidOperationException(string.Format(CliCommandStrings.UnexpectedMessageWithoutHandshake, nameof(TestResultMessages)));
}

var handshakeInfo = _handshakeInfo.Value;
Expand Down Expand Up @@ -158,13 +161,14 @@ internal void OnFileArtifactsReceived(FileArtifactMessages fileArtifactMessages)
{
LogFileArtifacts(fileArtifactMessages);

// TODO: If _handshakeInfo is null, we should error.
// We shouldn't be getting any file artifact messages without a previous handshake.

if (_options.IsHelp)
{
// TODO: Better to throw exception?
return;
throw new InvalidOperationException(string.Format(CliCommandStrings.UnexpectedMessageInHelpMode, nameof(FileArtifactMessages)));
}

if (!_handshakeInfo.HasValue)
{
throw new InvalidOperationException(string.Format(CliCommandStrings.UnexpectedMessageWithoutHandshake, nameof(FileArtifactMessages)));
}

var handshakeInfo = _handshakeInfo.Value;
Expand All @@ -183,8 +187,12 @@ internal void OnSessionEventReceived(TestSessionEvent sessionEvent)
{
LogSessionEvent(sessionEvent);

// TODO: If _handshakeInfo is null, we should error.
// We shouldn't be getting any session event messages without a previous handshake.
// TODO: Validate if we should get this message in help mode or not.

if (!_handshakeInfo.HasValue)
{
throw new InvalidOperationException(string.Format(CliCommandStrings.UnexpectedMessageWithoutHandshake, nameof(DiscoveredTestMessages)));
}

if (sessionEvent.SessionType == SessionEventTypes.TestSessionStart)
{
Expand Down
15 changes: 15 additions & 0 deletions src/Cli/dotnet/Commands/xlf/CliCommandStrings.cs.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions src/Cli/dotnet/Commands/xlf/CliCommandStrings.de.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions src/Cli/dotnet/Commands/xlf/CliCommandStrings.es.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions src/Cli/dotnet/Commands/xlf/CliCommandStrings.fr.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading