Skip to content

Issue 736 fix attempt experiment #754

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

Closed
MarcoRossignoli wants to merge 2 commits into from
Closed

Issue 736 fix attempt experiment #754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3def55d
sync hit file access
MarcoRossignoli Feb 24, 2020
5648377
distiguish environment
MarcoRossignoli Feb 25, 2020
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
3 changes: 2 additions & 1 deletion src/coverlet.collector/DataCollection/CoverageWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ public Coverage CreateCoverage(CoverletSettings settings, ILogger coverletLogger
settings.UseSourceLink,
coverletLogger,
DependencyInjection.Current.GetService<IInstrumentationHelper>(),
DependencyInjection.Current.GetService<IFileSystem>());
DependencyInjection.Current.GetService<IFileSystem>(),
true);
}

/// <summary>
Expand Down
8 changes: 4 additions & 4 deletions src/coverlet.collector/InProcDataCollection/CoverletInProcDataCollector.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,10 @@ public void TestSessionEnd(TestSessionEndArgs testSessionEndArgs)

try
{
_eqtTrace.Verbose($"Calling ModuleTrackerTemplate.UnloadModule for '{injectedInstrumentationClass.Assembly.FullName}'");
var unloadModule = injectedInstrumentationClass.GetMethod(nameof(ModuleTrackerTemplate.UnloadModule), new[] { typeof(object), typeof(EventArgs) });
unloadModule.Invoke(null, new[] { null, EventArgs.Empty });
_eqtTrace.Verbose($"Called ModuleTrackerTemplate.UnloadModule for '{injectedInstrumentationClass.Assembly.FullName}'");
_eqtTrace.Verbose($"Calling ModuleTrackerTemplate.InProcessCollectorFlush for '{injectedInstrumentationClass.Assembly.FullName}'");
var unloadModule = injectedInstrumentationClass.GetMethod(nameof(ModuleTrackerTemplate.InProcessCollectorFlush));
unloadModule.Invoke(null, new object[0]);
_eqtTrace.Verbose($"Called ModuleTrackerTemplate.InProcessCollectorFlush for '{injectedInstrumentationClass.Assembly.FullName}'");
}
catch (Exception ex)
{
Expand Down
18 changes: 16 additions & 2 deletions src/coverlet.core/Coverage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ internal class Coverage
private string[] _excludeAttributes;
private bool _includeTestAssembly;
private bool _singleHit;
private bool _isInstrumentedByOutOfProcessCollector;
private string _mergeWith;
private bool _useSourceLink;
private ILogger _logger;
Expand All @@ -45,7 +46,8 @@ public Coverage(string module,
bool useSourceLink,
ILogger logger,
IInstrumentationHelper instrumentationHelper,
IFileSystem fileSystem)
IFileSystem fileSystem,
bool isInstrumentedByOutOfProcessCollector = false)
{
_module = module;
_includeFilters = includeFilters;
Expand All @@ -55,6 +57,7 @@ public Coverage(string module,
_excludeAttributes = excludeAttributes;
_includeTestAssembly = includeTestAssembly;
_singleHit = singleHit;
_isInstrumentedByOutOfProcessCollector = isInstrumentedByOutOfProcessCollector;
_mergeWith = mergeWith;
_useSourceLink = useSourceLink;
_logger = logger;
Expand Down Expand Up @@ -97,7 +100,18 @@ public CoveragePrepareResult PrepareModules()
continue;
}

var instrumenter = new Instrumenter(module, _identifier, _excludeFilters, _includeFilters, _excludedSourceFiles, _excludeAttributes, _singleHit, _logger, _instrumentationHelper, _fileSystem);
var instrumenter = new Instrumenter(module,
_identifier,
_excludeFilters,
_includeFilters,
_excludedSourceFiles,
_excludeAttributes,
_singleHit,
_logger,
_instrumentationHelper,
_fileSystem,
_isInstrumentedByOutOfProcessCollector);

if (instrumenter.CanInstrument())
{
_instrumentationHelper.BackupOriginalModule(module, _identifier);
Expand Down
12 changes: 10 additions & 2 deletions src/coverlet.core/Instrumentation/Instrumenter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ internal class Instrumenter
private readonly ExcludedFilesHelper _excludedFilesHelper;
private readonly string[] _excludedAttributes;
private readonly bool _singleHit;
private readonly bool _isInstrumentedByOutOfProcessCollector;
private readonly bool _isCoreLibrary;
private readonly ILogger _logger;
private readonly IInstrumentationHelper _instrumentationHelper;
Expand All @@ -33,6 +34,7 @@ internal class Instrumenter
private FieldDefinition _customTrackerHitsArray;
private FieldDefinition _customTrackerHitsFilePath;
private FieldDefinition _customTrackerSingleHit;
private FieldDefinition _customIsCalledByInProcessCollector;
private ILProcessor _customTrackerClassConstructorIl;
private TypeDefinition _customTrackerTypeDef;
private MethodReference _customTrackerRegisterUnloadEventsMethod;
Expand All @@ -54,7 +56,8 @@ public Instrumenter(
bool singleHit,
ILogger logger,
IInstrumentationHelper instrumentationHelper,
IFileSystem fileSystem)
IFileSystem fileSystem,
bool isInstrumentedByOutOfProcessCollector = false)
{
_module = module;
_identifier = identifier;
Expand All @@ -63,6 +66,7 @@ public Instrumenter(
_excludedFilesHelper = new ExcludedFilesHelper(excludedFiles, logger);
_excludedAttributes = excludedAttributes;
_singleHit = singleHit;
_isInstrumentedByOutOfProcessCollector = isInstrumentedByOutOfProcessCollector;
_isCoreLibrary = Path.GetFileNameWithoutExtension(_module) == "System.Private.CoreLib";
_logger = logger;
_instrumentationHelper = instrumentationHelper;
Expand Down Expand Up @@ -240,6 +244,8 @@ private void InstrumentModule()
_customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerHitsFilePath));
_customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(_singleHit ? OpCodes.Ldc_I4_1 : OpCodes.Ldc_I4_0));
_customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerSingleHit));
_customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(_isInstrumentedByOutOfProcessCollector ? OpCodes.Ldc_I4_1 : OpCodes.Ldc_I4_0));
_customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Stsfld, _customIsCalledByInProcessCollector));

if (containsAppContext)
{
Expand All @@ -248,7 +254,7 @@ private void InstrumentModule()
// initialization of the custom tracker and the static initialization of the hosting AppDomain
// (which for the core library case will be instrumented code).
var eventArgsType = new TypeReference(nameof(System), nameof(EventArgs), module, module.TypeSystem.CoreLibrary);
var customTrackerUnloadModule = new MethodReference(nameof(ModuleTrackerTemplate.UnloadModule), module.TypeSystem.Void, _customTrackerTypeDef);
var customTrackerUnloadModule = new MethodReference(nameof(ModuleTrackerTemplate.AppContextOnProcessExitEvent), module.TypeSystem.Void, _customTrackerTypeDef);
customTrackerUnloadModule.Parameters.Add(new ParameterDefinition(module.TypeSystem.Object));
customTrackerUnloadModule.Parameters.Add(new ParameterDefinition(eventArgsType));

Expand Down Expand Up @@ -291,6 +297,8 @@ private void AddCustomModuleTrackerToModule(ModuleDefinition module)
_customTrackerHitsFilePath = fieldClone;
else if (fieldClone.Name == nameof(ModuleTrackerTemplate.SingleHit))
_customTrackerSingleHit = fieldClone;
else if (fieldClone.Name == nameof(ModuleTrackerTemplate.IsCalledByInProcessCollector))
_customIsCalledByInProcessCollector = fieldClone;
}

foreach (MethodDefinition methodDef in moduleTrackerTemplate.Methods)
Expand Down
40 changes: 36 additions & 4 deletions src/coverlet.core/Instrumentation/ModuleTrackerTemplate.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ internal static class ModuleTrackerTemplate
public static string HitsFilePath;
public static int[] HitsArray;
public static bool SingleHit;
public static bool IsCalledByInProcessCollector;
private static readonly bool _enableLog = int.TryParse(Environment.GetEnvironmentVariable("COVERLET_ENABLETRACKERLOG"), out int result) ? result == 1 : false;

static ModuleTrackerTemplate()
Expand All @@ -35,8 +36,8 @@ static ModuleTrackerTemplate()
// to UnloadModule will be injected in System.AppContext.OnProcessExit.
public static void RegisterUnloadEvents()
{
AppDomain.CurrentDomain.ProcessExit += new EventHandler(UnloadModule);
AppDomain.CurrentDomain.DomainUnload += new EventHandler(UnloadModule);
AppDomain.CurrentDomain.ProcessExit += new EventHandler(ProcessExitEvent);
AppDomain.CurrentDomain.DomainUnload += new EventHandler(DomainUnloadEvent);
}

public static void RecordHitInCoreLibrary(int hitLocationIndex)
Expand Down Expand Up @@ -73,11 +74,42 @@ public static void RecordSingleHit(int hitLocationIndex)
location = 1;
}

public static void UnloadModule(object sender, EventArgs e)
private static bool IsDotNetCore()
{
// object for .NET Framework is inside mscorlib.dll
return Path.GetFileName(typeof(object).Assembly.Location) == "System.Private.CoreLib.dll";
}

public static void ProcessExitEvent(object sender, EventArgs e)
{
if (IsCalledByInProcessCollector && IsDotNetCore())
Copy link

@abe545 abe545 Mar 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

At least for my test projects, moving the call to IsDotNetCore to https://github.com/tonerdo/coverlet/blob/master/src/coverlet.core/Instrumentation/ModuleTrackerTemplate.cs#L111 still solves my issue - and should address your concern about people who don't update their test sdk (#736 (comment)).

Copy link

@abe545 abe545 Mar 11, 2020

Choose a reason for hiding this comment

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

Basically, we could remove this whole if block, and replace
if (failedToCreateNewHitsFile) with if (failedToCreateNewHitsFile && !IsDotNetCore())
It might be a safer change?

Copy link
Collaborator Author

@MarcoRossignoli MarcoRossignoli Mar 12, 2020

Choose a reason for hiding this comment

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

At least for my test projects, moving the call to IsDotNetCore to https://github.com/tonerdo/coverlet/pull/754/files#diff-164040e68c2b496a18449c2363998152L111 still solves my issue - and should address your concern about people who don't update their test sdk (#736 (comment)).

Cannot understand where this link pointing...can you rewrite it?

Copy link
Collaborator Author

@MarcoRossignoli MarcoRossignoli Mar 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

if (failedToCreateNewHitsFile) with if (failedToCreateNewHitsFile && !IsDotNetCore())

This could work...I'm only scared about remove redundant "flush" on exit(I want to do it for a long time) because could hidden some latent bug that will lead to empty coverage file.
I'll take a look asap, busy weeks for me.
Thank's for investigation.

Copy link

@abe545 abe545 Mar 12, 2020

Choose a reason for hiding this comment

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

At least for my test projects, moving the call to IsDotNetCore to https://github.com/tonerdo/coverlet/pull/754/files#diff-164040e68c2b496a18449c2363998152L111 still solves my issue - and should address your concern about people who don't update their test sdk (#736 (comment)).

Cannot understand where this link pointing...can you rewrite it?

Updated it to point at the file outside this pr

Copy link

@abe545 abe545 Mar 17, 2020

Choose a reason for hiding this comment

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

And to hopefully add some clarification, I added a readme to that repo.
Let me try to pull your change with the mutex and build it in that repo (in a different branch) so I can demonstrate the issue.

Copy link

@abe545 abe545 Mar 17, 2020

Choose a reason for hiding this comment

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

Right, got it to fail intermittently using the mutex commit: https://github.com/namely/CoverletErrorRepro/tree/use-coverlet-mutex
As stated in the readme, this fails roughly 1/3 times on my laptop. I've noticed our CI environment is much slower than my laptop at running tests, so it is perhaps exacerbating the problem.

Copy link
Collaborator Author

@MarcoRossignoli MarcoRossignoli Mar 17, 2020

Choose a reason for hiding this comment

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

Thank's for sample and for correct exception, that fix doesn't work my fault...synch is working but if thread acquired mutex inside tracker is killed wait on get coverage will fail for abandoned mutex and also if we catch that exception we cannot be sure that file was fully written so we can hit corrupted hit file.

The unique clean solution is the one in full patch or your idea of skip re-write if we're in .net core app because if we already called flush from collector the second one should be process exits(no app domain so no intermediate flushes expected and only one file creation expected for msbuild/.net tool with .net core runtime).

Copy link

@abe545 abe545 Mar 17, 2020

Choose a reason for hiding this comment

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

No problem! Let me know if I can help with this in any way.

Copy link
Collaborator Author

@MarcoRossignoli MarcoRossignoli Mar 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Now we've a complete analysis, I'll think about best and light solution, for now you can fork and use your idea.
Huge thanks!

{
return;
}

Flush(nameof(ProcessExitEvent));
}

public static void DomainUnloadEvent(object sender, EventArgs e)
{
Flush(nameof(DomainUnloadEvent));
}

public static void AppContextOnProcessExitEvent(object sender, EventArgs e)
{
Flush(nameof(AppContextOnProcessExitEvent));
}

public static void InProcessCollectorFlush()
{
Flush(nameof(InProcessCollectorFlush));
}

private static void Flush(string flushType)
{
try
{
WriteLog($"Unload called for '{Assembly.GetExecutingAssembly().Location}'");
WriteLog($"Unload called for '{Assembly.GetExecutingAssembly().Location}' FlushType: {flushType}");
// Claim the current hits array and reset it to prevent double-counting scenarios.
int[] hitsArray = Interlocked.Exchange(ref HitsArray, new int[HitsArray.Length]);

Expand Down
2 changes: 1 addition & 1 deletion test/coverlet.core.tests/Coverage/InstrumenterHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ async public static Task<CoveragePrepareResult> Run<T>(Func<dynamic, Task> callM
// string hitsFilePath = (string)tracker.GetField("HitsFilePath").GetValue(null);

// Void UnloadModule(System.Object, System.EventArgs)
tracker.GetTypeInfo().GetMethod("UnloadModule").Invoke(null, new object[2] { null, null });
tracker.GetTypeInfo().GetMethod(nameof(ModuleTrackerTemplate.InProcessCollectorFlush)).Invoke(null, new object[0]);

// Persist CoveragePrepareResult
using (FileStream fs = new FileStream(persistPrepareResultToFile, FileMode.Open))
Expand Down
16 changes: 8 additions & 8 deletions test/coverlet.core.tests/Instrumentation/ModuleTrackerTemplateTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ public TrackerContext()
public void Dispose()
{
File.Delete(ModuleTrackerTemplate.HitsFilePath);
AppDomain.CurrentDomain.ProcessExit -= ModuleTrackerTemplate.UnloadModule;
AppDomain.CurrentDomain.DomainUnload -= ModuleTrackerTemplate.UnloadModule;
AppDomain.CurrentDomain.ProcessExit -= ModuleTrackerTemplate.ProcessExitEvent;
AppDomain.CurrentDomain.DomainUnload -= ModuleTrackerTemplate.DomainUnloadEvent;
}
}

Expand All @@ -34,7 +34,7 @@ public void HitsFileCorrectlyWritten()
{
using var ctx = new TrackerContext();
ModuleTrackerTemplate.HitsArray = new[] { 1, 2, 0, 3 };
ModuleTrackerTemplate.UnloadModule(null, null);
ModuleTrackerTemplate.InProcessCollectorFlush();

var expectedHitsArray = new[] { 1, 2, 0, 3 };
Assert.Equal(expectedHitsArray, ReadHitsFile());
Expand All @@ -51,7 +51,7 @@ public void HitsFileWithDifferentNumberOfEntriesCausesExceptionOnUnload()
using var ctx = new TrackerContext();
WriteHitsFile(new[] { 1, 2, 3 });
ModuleTrackerTemplate.HitsArray = new[] { 1 };
Assert.Throws<InvalidOperationException>(() => ModuleTrackerTemplate.UnloadModule(null, null));
Assert.Throws<InvalidOperationException>(() => ModuleTrackerTemplate.InProcessCollectorFlush());
return _success;
});
}
Expand All @@ -69,7 +69,7 @@ public void HitsOnMultipleThreadsCorrectlyCounted()
t.Start(i);
}

ModuleTrackerTemplate.UnloadModule(null, null);
ModuleTrackerTemplate.InProcessCollectorFlush();
var expectedHitsArray = new[] { 4, 3, 2, 1 };
Assert.Equal(expectedHitsArray, ReadHitsFile());

Expand All @@ -93,10 +93,10 @@ public void MultipleSequentialUnloadsHaveCorrectTotalData()
{
using var ctx = new TrackerContext();
ModuleTrackerTemplate.HitsArray = new[] { 0, 3, 2, 1 };
ModuleTrackerTemplate.UnloadModule(null, null);
ModuleTrackerTemplate.InProcessCollectorFlush();

ModuleTrackerTemplate.HitsArray = new[] { 0, 1, 2, 3 };
ModuleTrackerTemplate.UnloadModule(null, null);
ModuleTrackerTemplate.InProcessCollectorFlush();

var expectedHitsArray = new[] { 0, 4, 4, 4 };
Assert.Equal(expectedHitsArray, ReadHitsFile());
Expand All @@ -117,7 +117,7 @@ public void MutexBlocksMultipleWriters()
Assert.True(createdNew);

ModuleTrackerTemplate.HitsArray = new[] { 0, 1, 2, 3 };
var unloadTask = Task.Run(() => ModuleTrackerTemplate.UnloadModule(null, null));
var unloadTask = Task.Run(() => ModuleTrackerTemplate.InProcessCollectorFlush());

Assert.False(unloadTask.Wait(5));

Expand Down
1 change: 1 addition & 0 deletions test/coverlet.integration.tests/BaseTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ private protected bool RunCommand(string command, string arguments, out string s
psi.WorkingDirectory = workingDirectory;
psi.RedirectStandardError = true;
psi.RedirectStandardOutput = true;
psi.EnvironmentVariables.Add("COVERLET_ENABLETRACKERLOG", "1");
Process commandProcess = Process.Start(psi);
if (!commandProcess.WaitForExit((int)TimeSpan.FromMinutes(5).TotalMilliseconds))
{
Expand Down