Skip to content
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.

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

Coverlet in-proc data collector may crash with access violation because the type is null #2978

Open
KirillOsenkov opened this issue Jul 10, 2021 · 12 comments

Comments

@KirillOsenkov
Copy link
Member

We are investigating crashes during CI for an internal repo (VSLanguageServerClient), where the stack looks like this:

Microsoft.TestPlatform.CrossPlatEngine.dll!Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.DataCollection.InProcDataCollector.CreateObjectFromType Line 107	C#
>	Microsoft.TestPlatform.CrossPlatEngine.dll!Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.DataCollection.InProcDataCollectionExtensionManager.CreateDataCollector Line 75	C#
Microsoft.TestPlatform.CrossPlatEngine.dll!Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.DataCollection.InProcDataCollectionExtensionManager.InitializeInProcDataCollectors Line 124	C#
Microsoft.TestPlatform.CrossPlatEngine.dll!Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.DataCollection.InProcDataCollectionExtensionManager.InProcDataCollectionExtensionManager Line 61	C#
Microsoft.TestPlatform.CrossPlatEngine.dll!Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Execution.ExecutionManager.InitializeDataCollectors Line 156	C#
Microsoft.TestPlatform.CrossPlatEngine.dll!Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Execution.ExecutionManager.StartTestRun Line 67	C#
Microsoft.TestPlatform.CrossPlatEngine.dll!Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.TestRequestHandler.OnMessageReceived.AnonymousMethod__3 Line 350	C#
Microsoft.TestPlatform.CrossPlatEngine.dll!Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.TestRequestHandler..ctor.AnonymousMethod__20_2 Line 92	C#
Microsoft.TestPlatform.CoreUtilities.dll!Microsoft.VisualStudio.TestPlatform.Utilities.JobQueue<System.Action>.SafeProcessJob	Unknown
Microsoft.TestPlatform.CoreUtilities.dll!Microsoft.VisualStudio.TestPlatform.Utilities.JobQueue<System.Action>.BackgroundJobProcessor	Unknown
Microsoft.TestPlatform.CoreUtilities.dll!Microsoft.VisualStudio.TestPlatform.Utilities.JobQueue<System.__Canon>..ctor.AnonymousMethod__12_0	Unknown

The dataCollectorType is null here:

this.dataCollectorObject = CreateObjectFromType(dataCollectorType);

The way dataCollectorType is assigned using Reflection is flaky:

if (Path.GetFileName(codeBase) == Constants.CoverletDataCollectorCodebase)
{
// If we're loading coverlet collector we skip to check the version of assembly
// to allow upgrade through nuget package
filterPredicate = (x) => x.FullName.Equals(Constants.CoverletDataCollectorTypeName) && interfaceTypeInfo.IsAssignableFrom(x.GetTypeInfo());
// Coverlet collector is consumed as nuget package we need to add assemblies directory to resolver to correctly load references.
Debug.Assert(Path.IsPathRooted(codeBase), "Absolute path expected");
testPluginCache.AddResolverSearchDirectories(new string[] { Path.GetDirectoryName(codeBase) });
}

There is no validation if the type can't be found:

this.dataCollectorType = assembly?.GetTypes().FirstOrDefault(filterPredicate);
this.AssemblyQualifiedName = this.dataCollectorType?.AssemblyQualifiedName;

As a result the test run will crash and disappear with no symptoms other than the "Test host process crashed".

The dumps are available internally, ping KirillO.

@KirillOsenkov
Copy link
Member Author

@nohwnd @MarcoRossignoli

@KirillOsenkov
Copy link
Member Author

The data collector settings are:

<title>Document</title>
  Name Value Type
[0] {Microsoft.VisualStudio.TestPlatform.ObjectModel.DataCollectorSettings} Microsoft.VisualStudio.TestPlatform.ObjectModel.DataCollectorSettings
  AssemblyQualifiedName "Coverlet.Collector.DataCollection.CoverletInProcDataCollector, coverlet.collector, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null" string
  CodeBase "coverlet.collector.dll" string
  Configuration null System.Xml.XmlElement
  FriendlyName "XPlat Code Coverage" string
  IsEnabled true bool
  ▶ Uri null System.Uri

@KirillOsenkov
Copy link
Member Author

The runsettings file used was:

<RunSettings>
  <RunConfiguration>
    <BatchSize>1000</BatchSize>
    <ResultsDirectory>C:\a\_temp\TestResults</ResultsDirectory>
    <TargetPlatform>X86</TargetPlatform>
    <TargetFrameworkVersion>.NETFramework,Version=v4.7.2</TargetFrameworkVersion>
    <TestAdaptersPaths>C:\a\1\s</TestAdaptersPaths>
    <InIsolation>true</InIsolation>
    <DesignMode>False</DesignMode>
    <CollectSourceInformation>False</CollectSourceInformation>
  </RunConfiguration>
  <DataCollectionRunSettings>
    <DataCollectors>
      <DataCollector friendlyName="blame" enabled="True">
        <Configuration>
          <ResultsDirectory>C:\a\_temp\TestResults</ResultsDirectory>
          <CollectDump CollectAlways="true" />
          <CollectDump />
        </Configuration>
      </DataCollector>
      <DataCollector friendlyName="XPlat Code Coverage" enabled="True" />
    </DataCollectors>
  </DataCollectionRunSettings>
  <LoggerRunSettings>
    <Loggers>
      <Logger friendlyName="blame" enabled="True" />
      <Logger friendlyName="trx" enabled="True" />
      <Logger friendlyName="Console" uri="logger://microsoft/TestPlatform/ConsoleLogger/v1" assemblyQualifiedName="Microsoft.VisualStudio.TestPlatform.CommandLine.Internal.ConsoleLogger, vstest.console, Version=15.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" codeBase="C:\a\_tool\VsTest\17.0.0-preview-20210709-01\x64\tools\net451\Common7\IDE\Extensions\TestPlatform\vstest.console.exe" enabled="True" />
    </Loggers>
  </LoggerRunSettings>
  <RunSettingsDirectory>C:\a\_temp</RunSettingsDirectory>
  <InProcDataCollectionRunSettings>
    <InProcDataCollectors>
      <InProcDataCollector assemblyQualifiedName="Coverlet.Collector.DataCollection.CoverletInProcDataCollector, coverlet.collector, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null" friendlyName="XPlat Code Coverage" enabled="True" codebase="coverlet.collector.dll" />
    </InProcDataCollectors>
  </InProcDataCollectionRunSettings>
</RunSettings>

@KirillOsenkov
Copy link
Member Author

KirillOsenkov commented Jul 10, 2021

I'm guessing that coverlet.collector.dll isn't loaded or found. Note the version of coverlet.collector.dll in RunSettings is 1.0.0.0, but the actual version on my machine is 1.3.0.0.

It needs to do more detailed logging here:

assembly = this.assemblyLoadContext.LoadAssemblyFromPath(Environment.ExpandEnvironmentVariables(codeBase));

The PATH environment variable contents is:

C:\agent\externals\git\cmd
C:\agent\externals\git\mingw64\bin
C:\a\_tool/dotnet
C:\a\_tool\dotnet\
C:\Program Files (x86)\IncrediBuild
C:\Program Files (x86)\NVIDIA Corporation\PhysX\Common
C:\Program Files (x86)\Windows Kits\10\Windows Performance Toolkit\
C:\Program Files\CMake\bin
C:\Program Files\Docker
C:\Program Files\dotnet\
C:\Program Files\Git LFS
C:\Program Files\Git\cmd
C:\Program Files\Microsoft SQL Server\130\Tools\Binn\
C:\Program Files\Microsoft SQL Server\Client SDK\ODBC\170\Tools\Binn\
C:\Program Files\nodejs\
C:\Program Files\NVIDIA Corporation\Nsight Compute 2020.3.1\
C:\Program Files\NVIDIA GPU Computing Toolkit\CUDA\v11.2\bin
C:\Program Files\NVIDIA GPU Computing Toolkit\CUDA\v11.2\libnvvp
C:\Program Files\PowerShell\7\
C:\ProgramData\chocolatey\bin
C:\ProgramData\chocolatey\lib\maven\apache-maven-3.8.1\bin
C:\Python37\
C:\Python37\Scripts\
C:\Users\AzDevOps\AppData\Local\Microsoft\WindowsApps
C:\Windows
C:\Windows\system32
C:\Windows\System32\OpenSSH\
C:\Windows\System32\Wbem
C:\Windows\System32\WindowsPowerShell\v1.0\

Not sure where it's supposed to find coverlet.collector.dll from.

@KirillOsenkov
Copy link
Member Author

@KirillOsenkov
Copy link
Member Author

Is there no way to disable the in-proc coverlet collector? Why is it running? We're not passing --collect:"XPlat Code Coverage" as far as I can see. Why is it enabled in the dump?

@KirillOsenkov
Copy link
Member Author

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Jul 10, 2021

Hi @KirillOsenkov @nohwnd

Can you try to remove from runsettings the section InProcDataCollectionRunSettings and pass as argument --collect:"XPlat Code Coverage"? Can you enable the logs https://github.com/coverlet-coverage/coverlet/blob/master/Documentation/Troubleshooting.md#collectors-integration , usually host logs contains collectors loading issues and we can check the actual runsettings used.

Coverlet collectors(out/in process) were introduced as workaround for the 100ms issue.

First version of Coverlet was implemented with msbuild tasks and it's supported yet.
Coverlet persists hits temporary in a memory array of int and when the event process exit/unload app domain fire it writes these to a tmp file on file system(temp dir). This file is used to generate the report.
Now due to the 100ms issue sometime happened that VSTest plat kills the process before Coverlet is able to persist this file ending with an empty report.

For this reason @vagisha-nidhi wrote the first version of collectors. Out of process collector does the instrumentation as usual, in-process collector ensure that flush to file be done. Adding the in-process collector is the key to orchestrate the "shutdown" in a correct way being "orchestrated" by VSTest platform.

Current implementation inside VSTest injects the inprocess collector during startup with a custom argument processor. The result is something like https://github.com/microsoft/vstest/blob/main/test/vstest.console.UnitTests/Processors/CollectArgumentProcessorTests.cs#L263

The problem could be that the inprocess collector section is created manually and the codebase is not the path the adapter and so collector dll is not found. The path is created by processor here https://github.com/microsoft/vstest/blob/main/src/vstest.console/Processors/CollectArgumentProcessor.cs#L131 and should point to local nuget folder.
In process section is not supposed to be created manually.

The code below was added because we cannot use the assembly qualified name with a "fixed version"(I discovered this bug after first release) and so we need to search the type manually and to avoid to load types on runsettings in-proc injection generation inside argument processor we fixed here

if (Path.GetFileName(codeBase) == Constants.CoverletDataCollectorCodebase)
{
// If we're loading coverlet collector we skip to check the version of assembly
// to allow upgrade through nuget package
filterPredicate = (x) => x.FullName.Equals(Constants.CoverletDataCollectorTypeName) && interfaceTypeInfo.IsAssignableFrom(x.GetTypeInfo());
// Coverlet collector is consumed as nuget package we need to add assemblies directory to resolver to correctly load references.
Debug.Assert(Path.IsPathRooted(codeBase), "Absolute path expected");
testPluginCache.AddResolverSearchDirectories(new string[] { Path.GetDirectoryName(codeBase) });
}

the 100 ms timeout issue again

The reason to move to 30sec is to prevent this issue and in that case we could avoid to use collectors at all and stick with msbuild that supports also more feature because is not limited by VSTest rules/semantics(it's not clear to me why it's not possible in case of stop click on ide exit immediately with a cancellation token source or something like that, but I don't have any idea on current implementation so I think there is a good reason).

Another planned thing is to change completely how coverlet collect the hits and use a shared memory with a sidecar process so in case of crash we don't lose the hits. But this change is "huge" and time consuming. FYI some details here coverlet-coverage/coverlet#808

@KirillOsenkov
Copy link
Member Author

Related: #2599 (comment)

@KirillOsenkov
Copy link
Member Author

I have made a PR that adds a PackageReference to coverlet.collector, Version 3.0.3 to our projects. I think this could be a problem, if you're not referencing coverlet.collector, then the coverlet.collector.dll is simply missing from the output directory, and causing a problem in the static constructor.

I think one thing that can be improved is a better error message if coverlet.collector can't be found.

@KirillOsenkov
Copy link
Member Author

Also we don't have a RunSettings file at all, the RunSettings XML that I pasted earlier I'm guessing was the default or generated one I think?

Interesting why the in-proc collector was enabled by default.

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Jul 16, 2021

Interesting why the in-proc collector was enabled by default.

Mmm I think that this could happen if we're passing in some way --collect:"XPlat Code Coverage" but the package is not referenced. I need to repro to understand if command is parsed correctly and inproc collector is injected(added to runsettings) with wrong base path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants