Ensure dotnet test messages are received in the right timings/modes #50727
Conversation
| try | ||
| { | ||
| testAppPipeConnectionLoop = Task.Run(async () => await WaitConnectionAsync(cancellationToken), cancellationToken); | ||
| var testProcessExitCode = await StartProcess(processStartInfo); |
There was a problem hiding this comment.
should we pass cancellationToken to this? it seems that once we start the process we do not cancel here?
There was a problem hiding this comment.
Good question. It's very possible that cancellation doesn't work very well today. @mariam-abdulla Do you know please?
There was a problem hiding this comment.
Let's follow-up in a separate issue either way. #50732
There was a problem hiding this comment.
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.
| { | ||
| throw new InvalidOperationException(CliCommandStrings.MissingTestSessionEnd); | ||
| cancellationTokenSource.Cancel(); | ||
| testAppPipeConnectionLoop?.Wait((int)TimeSpan.FromSeconds(30).TotalMilliseconds); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Following-up in #50733
There was a problem hiding this comment.
I go agree that it's expected for this task to complete almost immediately.
There was a problem hiding this comment.
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.
| @@ -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)!; | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
We default to Environment.ProcessorCount unless specified by the user.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
| } | ||
| else | ||
| else if (_handshakeInfo.Value != currentHandshakeInfo) |
There was a problem hiding this comment.
under what circumstances do we expect multiple handshakes?
There was a problem hiding this comment.
TestHostController+TestHost, or an orchestrator (e.g, Retry, or a future sharding orchestrator)
No description provided.