From 80af388e851d0d8b88f93d07d46d47054acb9951 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Tue, 13 Aug 2024 11:16:37 +0200 Subject: [PATCH] Wait for Discovery to initialize before Cancelling it (#5177) --- .../TestPlatformHelpers/TestRequestManager.cs | 194 ++++++++++-------- 1 file changed, 110 insertions(+), 84 deletions(-) diff --git a/src/vstest.console/TestPlatformHelpers/TestRequestManager.cs b/src/vstest.console/TestPlatformHelpers/TestRequestManager.cs index d95891c425..f93857b671 100644 --- a/src/vstest.console/TestPlatformHelpers/TestRequestManager.cs +++ b/src/vstest.console/TestPlatformHelpers/TestRequestManager.cs @@ -78,6 +78,13 @@ internal class TestRequestManager : ITestRequestManager /// Assumption: There can only be one active discovery request. /// private IDiscoveryRequest? _currentDiscoveryRequest; + /// + /// Guards cancellation of the current discovery request, by resetting when the request is received, + /// because the request needs time to setup and populate the _currentTestRunRequest. This might take a relatively + /// long time when the machine is slow, because the setup is called as an async task, so it needs to be processed by thread pool + /// and there might be a queue of existing tasks. + /// + private readonly ManualResetEvent _discoveryStarting = new(true); /// /// Maintains the current active test run attachments processing cancellation token source. @@ -163,106 +170,119 @@ public void DiscoverTests( ITestDiscoveryEventsRegistrar discoveryEventsRegistrar, ProtocolConfig protocolConfig) { - EqtTrace.Info("TestRequestManager.DiscoverTests: Discovery tests started."); - - // TODO: Normalize rest of the data on the request as well - discoveryPayload.Sources = KnownPlatformSourceFilter.FilterKnownPlatformSources(discoveryPayload.Sources?.Distinct().ToList()); - discoveryPayload.RunSettings ??= ""; - - var runsettings = discoveryPayload.RunSettings; - - if (discoveryPayload.TestPlatformOptions != null) + try { - _telemetryOptedIn = discoveryPayload.TestPlatformOptions.CollectMetrics; - } + // Flag that that discovery is being initialized, so all requests to cancel discovery will wait till we set the discovery up. + _discoveryStarting.Reset(); - var requestData = GetRequestData(protocolConfig); - if (UpdateRunSettingsIfRequired( - runsettings, - discoveryPayload.Sources.ToList(), - discoveryEventsRegistrar, - isDiscovery: true, - out string updatedRunsettings, - out IDictionary sourceToArchitectureMap, - out IDictionary sourceToFrameworkMap)) - { - runsettings = updatedRunsettings; - } + // Make sure to run the run request inside a lock as the below section is not thread-safe. + // There can be only one discovery or execution request at a given point in time. + lock (_syncObject) + { + EqtTrace.Info("TestRequestManager.DiscoverTests: Discovery tests started."); - var sourceToSourceDetailMap = discoveryPayload.Sources.Select(source => new SourceDetail - { - Source = source, - Architecture = sourceToArchitectureMap[source], - Framework = sourceToFrameworkMap[source], - }).ToDictionary(k => k.Source!); + // TODO: Normalize rest of the data on the request as well + discoveryPayload.Sources = KnownPlatformSourceFilter.FilterKnownPlatformSources(discoveryPayload.Sources?.Distinct().ToList()); + discoveryPayload.RunSettings ??= ""; - var runConfiguration = XmlRunSettingsUtilities.GetRunConfigurationNode(runsettings); - var batchSize = runConfiguration.BatchSize; - var testCaseFilterFromRunsettings = runConfiguration.TestCaseFilter; + var runsettings = discoveryPayload.RunSettings; - if (requestData.IsTelemetryOptedIn) - { - // Collect metrics. - CollectMetrics(requestData, runConfiguration); + if (discoveryPayload.TestPlatformOptions != null) + { + _telemetryOptedIn = discoveryPayload.TestPlatformOptions.CollectMetrics; + } - // Collect commands. - LogCommandsTelemetryPoints(requestData); - } + var requestData = GetRequestData(protocolConfig); + if (UpdateRunSettingsIfRequired( + runsettings, + discoveryPayload.Sources.ToList(), + discoveryEventsRegistrar, + isDiscovery: true, + out string updatedRunsettings, + out IDictionary sourceToArchitectureMap, + out IDictionary sourceToFrameworkMap)) + { + runsettings = updatedRunsettings; + } - // Create discovery request. - var criteria = new DiscoveryCriteria( - discoveryPayload.Sources, - batchSize, - _commandLineOptions.TestStatsEventTimeout, - runsettings, - discoveryPayload.TestSessionInfo) - { - TestCaseFilter = _commandLineOptions.TestCaseFilterValue - ?? testCaseFilterFromRunsettings - }; + var sourceToSourceDetailMap = discoveryPayload.Sources.Select(source => new SourceDetail + { + Source = source, + Architecture = sourceToArchitectureMap[source], + Framework = sourceToFrameworkMap[source], + }).ToDictionary(k => k.Source!); - // Make sure to run the run request inside a lock as the below section is not thread-safe. - // There can be only one discovery or execution request at a given point in time. - lock (_syncObject) - { - try - { - EqtTrace.Info("TestRequestManager.DiscoverTests: Synchronization context taken"); + var runConfiguration = XmlRunSettingsUtilities.GetRunConfigurationNode(runsettings); + var batchSize = runConfiguration.BatchSize; + var testCaseFilterFromRunsettings = runConfiguration.TestCaseFilter; - _currentDiscoveryRequest = _testPlatform.CreateDiscoveryRequest( - requestData, - criteria, - discoveryPayload.TestPlatformOptions, - sourceToSourceDetailMap, - new EventRegistrarToWarningLoggerAdapter(discoveryEventsRegistrar)); - discoveryEventsRegistrar?.RegisterDiscoveryEvents(_currentDiscoveryRequest); + if (requestData.IsTelemetryOptedIn) + { + // Collect metrics. + CollectMetrics(requestData, runConfiguration); - // Notify start of discovery start. - _testPlatformEventSource.DiscoveryRequestStart(); + // Collect commands. + LogCommandsTelemetryPoints(requestData); + } - // Start the discovery of tests and wait for completion. - _currentDiscoveryRequest.DiscoverAsync(); - _currentDiscoveryRequest.WaitForCompletion(); - } - finally - { - if (_currentDiscoveryRequest != null) + // Create discovery request. + var criteria = new DiscoveryCriteria( + discoveryPayload.Sources, + batchSize, + _commandLineOptions.TestStatsEventTimeout, + runsettings, + discoveryPayload.TestSessionInfo) { - // Dispose the discovery request and unregister for events. - discoveryEventsRegistrar?.UnregisterDiscoveryEvents(_currentDiscoveryRequest); - _currentDiscoveryRequest.Dispose(); - _currentDiscoveryRequest = null; - } + TestCaseFilter = _commandLineOptions.TestCaseFilterValue + ?? testCaseFilterFromRunsettings + }; - EqtTrace.Info("TestRequestManager.DiscoverTests: Discovery tests completed."); - _testPlatformEventSource.DiscoveryRequestStop(); - // Posts the discovery complete event. - _metricsPublisher.Result.PublishMetrics( - TelemetryDataConstants.TestDiscoveryCompleteEvent, - requestData.MetricsCollection.Metrics!); + try + { + EqtTrace.Info("TestRequestManager.DiscoverTests: Synchronization context taken"); + + _currentDiscoveryRequest = _testPlatform.CreateDiscoveryRequest( + requestData, + criteria, + discoveryPayload.TestPlatformOptions, + sourceToSourceDetailMap, + new EventRegistrarToWarningLoggerAdapter(discoveryEventsRegistrar)); + // Discovery started, allow cancellations to proceed. + _currentDiscoveryRequest.OnDiscoveryStart += (s, e) => _discoveryStarting.Set(); + discoveryEventsRegistrar?.RegisterDiscoveryEvents(_currentDiscoveryRequest); + + // Notify start of discovery start. + _testPlatformEventSource.DiscoveryRequestStart(); + + // Start the discovery of tests and wait for completion. + _currentDiscoveryRequest.DiscoverAsync(); + _currentDiscoveryRequest.WaitForCompletion(); + } + finally + { + if (_currentDiscoveryRequest != null) + { + // Dispose the discovery request and unregister for events. + discoveryEventsRegistrar?.UnregisterDiscoveryEvents(_currentDiscoveryRequest); + _currentDiscoveryRequest.Dispose(); + _currentDiscoveryRequest = null; + } + + EqtTrace.Info("TestRequestManager.DiscoverTests: Discovery tests completed."); + _testPlatformEventSource.DiscoveryRequestStop(); + + // Posts the discovery complete event. + _metricsPublisher.Result.PublishMetrics( + TelemetryDataConstants.TestDiscoveryCompleteEvent, + requestData.MetricsCollection.Metrics!); + } } } + finally + { + _discoveryStarting.Set(); + } } /// @@ -624,6 +644,12 @@ public void CancelTestRun() public void CancelDiscovery() { EqtTrace.Info("TestRequestManager.CancelDiscovery: Sending cancel request."); + + // Wait for discovery request to initialize, before cancelling it, otherwise the + // _currentDiscoveryRequest might be null, because discovery did not have enough time to + // initialize and did not manage to populate _currentDiscoveryRequest yet, leading to hanging run + // that "ignores" the cancellation. + _discoveryStarting.WaitOne(3000); _currentDiscoveryRequest?.Abort(); }