Fix #48579 #49387

Ap0k · 2021-03-09T19:58:33Z

Fix issue #48579

stephentoub · 2021-03-09T22:42:02Z

src/tests/Loader/ContextualReflection/ContextualReflection.cs


            using (assemblyLoadContext.EnterContextualReflection())
            {
-                AssemblyBuilder assemblyBuilder = AssemblyBuilder.DefineDynamicAssembly(dynamicAssemblyName, assemblyBuilderAccess);
+                assemblyBuilder = AssemblyBuilder.DefineDynamicAssembly(dynamicAssemblyName, assemblyBuilderAccess);


I don't understand the change... what is this fixing?

The AssemblyBuilder was in the local scope of the using statement. If a dynamic assembly is created with the AssemblyBuilderAccess.RunAndCollect parameter, this could lead to the fact that at the time of checking the Assert condition it was already removed from memory. I think changing the scope of the AsemblyBuilder instance will solve the problem of floating test failure.

assemblyBuilder is never used; in a release build, these should produce exactly the same IL. Even if this fixes it for a debug build, where this could extend the lifetime of the instance until after the using block and prevent it from being collected before then, is this test never run in a release build?

Yes, the IL code was the same. Rewrote the test so that the AssemblyBuilder instance is used in the Assert test. The local load test did not issue any errors for an hour.

kunalspathak · 2021-04-01T19:47:40Z

Please revert the change in #50592 as part of this PR.

Ap0k · 2021-04-20T08:00:03Z

@kunalspathak Who else can review this PR?

kunalspathak · 2021-04-20T17:22:17Z

@kunalspathak Who else can review this PR?

@stephentoub has already reviewed and asked question. Could you please answer and suggest a better fix?

Ap0k · 2021-05-11T11:13:40Z

@stephentoub has already reviewed and asked question. Could you please answer and suggest a better fix?

@kunalspathak @stephentoub The question was answered, but no feedback was received.

kunalspathak · 2021-05-11T15:12:20Z

@Ap0k - Sorry for the delay. I have started various CI stress pipelines which will verify if your fix works.

Ap0k · 2021-05-11T15:38:58Z

@kunalspathak Okay. Waiting for test results.

kunalspathak · 2021-05-11T19:11:48Z

Looks like the test still fails for same reason, so it might not be the right fix. https://dev.azure.com/dnceng/public/_build/results?buildId=1132503&view=ms.vss-test-web.build-test-results-tab&runId=34375692&resultId=109547&paneView=dotnet-dnceng.dnceng-build-release-tasks.helix-test-information-tab

Ap0k · 2021-05-11T19:16:52Z

Looks like the test still fails for same reason, so it might not be the right fix. https://dev.azure.com/dnceng/public/_build/results?buildId=1132503&view=ms.vss-test-web.build-test-results-tab&runId=34375692&resultId=109547&paneView=dotnet-dnceng.dnceng-build-release-tasks.helix-test-information-tab

@kunalspathak Can you provide instructions for running a stress test locally? Maybe in my last launch I did not take into account any parameters.

kunalspathak · 2021-05-11T19:21:34Z

@kunalspathak Can you provide instructions for running a stress test locally? Maybe in my last launch I did not take into account any parameters.

Sure, you need to set the following environment variable. The failure is on Windows x64.

set COMPlus_TieredCompilation=0
set COMPlus_GCStress=0xC

Let me know if you need anything else.

Ap0k · 2021-05-11T19:25:23Z

Let me know if you need anything else.

Is it necessary to somehow change the configuration in testmix_gc.config?

kunalspathak · 2021-05-11T19:52:52Z

Let me know if you need anything else.

Is it necessary to somehow change the configuration in testmix_gc.config?

No, that should not be needed.

Ap0k · 2021-05-12T08:17:01Z

Fixed a bug with the definition of ALC for dynamic assembly;
The test logic has been changed. Now, instead of searching for an assembly in ALC by its name, its load context is determined and compared with the reference one.

Local test results:

=== TEST EXECUTION SUMMARY ===
   baseservices.compilerservices.XUnitWrapper           Total:   2, Errors: 0, Failed: 0, Skipped: 0, Time:    8,620s
   baseservices.exceptions.XUnitWrapper                 Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:   67,907s
   baseservices.TieredCompilation.XUnitWrapper          Total:  13, Errors: 0, Failed: 0, Skipped: 0, Time:   52,867s
   baseservices.typeequivalence.XUnitWrapper            Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:    4,692s
   baseservices.varargs.XUnitWrapper.dll                Total:   0
   CoreMangLib.system.XUnitWrapper                      Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:    3,572s
   Exceptions.ForeignThread.XUnitWrapper                Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:    3,959s
   GC.API.XUnitWrapper                                  Total:  32, Errors: 0, Failed: 0, Skipped: 0, Time:  199,606s
   GC.Coverage.XUnitWrapper                             Total:   2, Errors: 0, Failed: 0, Skipped: 0, Time:    8,613s
   GC.Features.XUnitWrapper                             Total:  21, Errors: 0, Failed: 0, Skipped: 0, Time:  186,054s
   GC.LargeMemory.XUnitWrapper                          Total:   3, Errors: 0, Failed: 0, Skipped: 0, Time:    6,566s
   GC.Regressions.XUnitWrapper                          Total:  10, Errors: 0, Failed: 0, Skipped: 0, Time:   20,874s
   GC.Scenarios.XUnitWrapper                            Total:  37, Errors: 0, Failed: 0, Skipped: 0, Time:  233,191s
   GC.Stress.XUnitWrapper                               Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:    0,347s
   ilasm.PortablePdb.XUnitWrapper                       Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:   17,226s
   ilasm.System.XUnitWrapper                            Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:    7,005s
   Loader.AssemblyDependencyResolver.XUnitWrapper       Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:    4,798s
   Loader.AssemblyLoadContext30Extensions.XUnitWrapper  Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:    3,580s
   Loader.binding.XUnitWrapper                          Total:   2, Errors: 0, Failed: 0, Skipped: 0, Time:  101,780s
   Loader.CollectibleAssemblies.XUnitWrapper            Total:   2, Errors: 0, Failed: 0, Skipped: 0, Time:    4,069s
   Loader.ContextualReflection.XUnitWrapper             Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:    5,234s
   Loader.regressions.XUnitWrapper                      Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:    3,575s
   managed.Compilation.XUnitWrapper                     Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:   74,885s
   readytorun.coreroot_determinism.XUnitWrapper         Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:  418,934s
   readytorun.crossgen2.XUnitWrapper                    Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:    8,234s
   readytorun.determinism.XUnitWrapper                  Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:   77,236s
   readytorun.DynamicMethodGCStress.XUnitWrapper.dll    Total:   0
   readytorun.r2rdump.XUnitWrapper                      Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:   19,578s
   readytorun.tests.XUnitWrapper                        Total:   3, Errors: 0, Failed: 0, Skipped: 0, Time:    5,054s
   reflection.DefaultInterfaceMethods.XUnitWrapper      Total:   3, Errors: 0, Failed: 0, Skipped: 0, Time:    3,812s
   reflection.Modifiers.XUnitWrapper                    Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:    3,600s
   reflection.RefEmit.XUnitWrapper                      Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:    3,787s
   reflection.regression.XUnitWrapper                   Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:    3,420s
   reflection.SetValue.XUnitWrapper                     Total:   2, Errors: 0, Failed: 0, Skipped: 0, Time:    5,323s
   reflection.StaticInterfaceMembers.XUnitWrapper       Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:    3,465s
   Regressions.coreclr.XUnitWrapper                     Total:  23, Errors: 0, Failed: 0, Skipped: 0, Time:   15,006s
   tracing.eventactivityidcontrol.XUnitWrapper          Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:    3,641s
   tracing.eventcounter.XUnitWrapper                    Total:   7, Errors: 0, Failed: 0, Skipped: 0, Time:    0,699s
   tracing.eventlistener.XUnitWrapper                   Total:   1, Errors: 0, Failed: 0, Skipped: 0, Time:    3,748s
   tracing.eventpipe.XUnitWrapper                       Total:  13, Errors: 0, Failed: 0, Skipped: 0, Time:   12,297s
                                                               ---          -          -           -        ---------
                                                  GRAND TOTAL: 197          0          0           0        1606,854s (1618,440s)

kunalspathak · 2021-05-13T06:29:30Z

I have kicked off another GCStress run.

Fixed a bug with the definition of ALC for dynamic assembly;

I do not fully understand this fix and might need to check with @stephentoub on what he thinks.

The test logic has been changed. Now, instead of searching for an assembly in ALC by its name, its load context is determined and compared with the reference one.

Why is the test changed? Was earlier the test was not validating the appropriate behaviour?

Ap0k · 2021-05-13T07:32:10Z

Why is the test changed? Was earlier the test was not validating the appropriate behaviour?

In the previous version of the test, a dynamic assembly with the same name was created for different ALCs. This could lead to incorrect test behavior. For example, first of all, create an assembly in ALC.Default, a search took place and the test completed successfully. Then a dynamic assembly was created for the unloaded ALC, but a search by name led to the fact that the assembly was taken from ALC.Default (this is due to the peculiarities of ALC.Assemblies) and the test failed.
That is why we had to fix the mentioned error and correct the logic of the test. Since it is quite logical that if we check the creation of a dynamic assembly for the selected ALC, then it is easier and faster after its creation to try to find it in this ALC, and not iterate over all assemblies from the context.

Ap0k · 2021-05-13T16:21:56Z

@kunalspathak what are the test results? I see from the logs that the test we are interested in has passed :)

kunalspathak · 2021-05-13T17:15:30Z

@kunalspathak what are the test results? I see from the logs that the test we are interested in has passed :)

There are lot of other failures that were not there before your changes. I have kicked off another CI job for main to double check if they are related to your changes or not. Could you also check some of the failures and see if they are related to your change?

Ap0k · 2021-05-13T17:25:30Z

@kunalspathak Ok, I'll check

kunalspathak · 2021-05-13T21:47:15Z

@kunalspathak Ok, I'll check

From the latest main CI runs, it looks like all those test also fail on main, so might not be related to your change. @stephentoub - do you mind reviewing the PR once again?

Ap0k · 2021-05-24T11:24:48Z

@kunalspathak Who else can review this PR?

kunalspathak · 2021-05-24T15:25:57Z

@agocke @vitek-karas @VSadov - Can someone review this PR?

vitek-karas · 2021-05-24T15:59:23Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.CoreCLR.cs

@@ -147,7 +147,7 @@ private static IntPtr ResolveUnmanagedDllUsingEvent(string unmanagedDllName, Ass

            AssemblyLoadContext? loadContextForAssembly = null;

-            RuntimeAssembly? rtAsm = assembly as RuntimeAssembly;
+            RuntimeAssembly? rtAsm = GetRuntimeAssembly(assembly);


This is a great fix - thanks a lot for finding this.

That said I would prefer if we made this a separate PR from the test fix. In that PR we should also add tests for this fix. My understanding is that it fixes two observable things:

The fact that GetLoadContext returns null for dynamic assemblies

The fact that dynamic assemblies don't show up in AssemblyLoadContext.Assemblies (even though they do show up in AppDomain.GetAssemblies)

Both of these should have tests (which are apparently missing).

After that PR is in, then we can fix the test which is the purpose of this PR.

Ok, I'll put this fix in a separate PR with the appropriate tests.

The same bug exists in ValidateAssemblyNameWithSimpleName. I filed a new issue for it: #53181

But it might be a good idea to fix both at the same time.

No problem) I'll do it

vitek-karas · 2021-05-24T16:00:13Z

src/tests/Loader/ContextualReflection/ContextualReflection.cs

            }

-            Assert.IsTrue(assemblyLoadContext.Assemblies.Any(a => AssemblyName.ReferenceMatchesDefinition(a.GetName(), dynamicAssemblyName)));
+            Assert.IsTrue(assemblyLoadContext == AssemblyLoadContext.GetLoadContext(assemblyBuilder));


With the fix in CoreLib I would expect both approaches to work - the comparison of ALC as well as searching for the assembly in ALC.Assemblies. Is that true?

Yes, provided that the name of the dynamic assembly is unique for each context (default and unloaded).

Returned the search for dynamic assembly in ALC.Assemblies. The dynamic assembly name is unique for each ALC.

vitek-karas · 2021-05-26T19:01:57Z

src/tests/Loader/ContextualReflection/ContextualReflection.cs


            using (assemblyLoadContext.EnterContextualReflection())
            {
-                AssemblyBuilder assemblyBuilder = AssemblyBuilder.DefineDynamicAssembly(dynamicAssemblyName, assemblyBuilderAccess);
+                assemblyBuilder = AssemblyBuilder.DefineDynamicAssembly(dynamicAssemblyName, assemblyBuilderAccess);
            }

            Assert.IsTrue(assemblyLoadContext.Assemblies.Any(a => AssemblyName.ReferenceMatchesDefinition(a.GetName(), dynamicAssemblyName)));


I think it should make use of the assemblyBuilder outside of the using block - just to be on the safe side. Otherwise the compiler/JIT can throw it away before it gets to the validation.

vitek-karas

LGTM - Thanks a lot!

vitek-karas · 2021-05-27T08:28:55Z

I restarted some of the CI legs as there were issues with AzDo.

AndyAyersMS · 2021-05-27T15:21:47Z

You can ignore the "pgo" failures -- for a short time these new pipelines were running on all PRs.

vitek-karas · 2021-05-28T18:42:29Z

Thank @Ap0k for the fix!

dotnet-issue-labeler bot added the area-System.Reflection-coreclr label Mar 9, 2021

stephentoub reviewed Mar 9, 2021

View reviewed changes

JulieLeeMSFT assigned Ap0k Mar 29, 2021

BruceForstall mentioned this pull request Apr 1, 2021

Disable TestDynamicAssembly tests #50592

Merged

Ap0k force-pushed the bugfix/ISSUE-48579 branch from c3738dd to ead09a1 Compare April 7, 2021 10:48

Ap0k requested a review from stephentoub April 7, 2021 15:42

Ap0k force-pushed the bugfix/ISSUE-48579 branch from ead09a1 to 3c57401 Compare May 12, 2021 08:09

vitek-karas reviewed May 24, 2021

View reviewed changes

vitek-karas mentioned this pull request May 24, 2021

AssemblyLoadContext incorrectly validates name of dynamic assembly when returned from Load #53181

Closed

Ap0k force-pushed the bugfix/ISSUE-48579 branch from 3c57401 to 0b225b7 Compare May 26, 2021 18:52

Ap0k requested a review from vitek-karas May 26, 2021 18:56

vitek-karas reviewed May 26, 2021

View reviewed changes

Fix dotnet#48579

48d24ec

Ap0k force-pushed the bugfix/ISSUE-48579 branch from 0b225b7 to 48d24ec Compare May 27, 2021 04:30

Ap0k requested a review from vitek-karas May 27, 2021 04:31

vitek-karas approved these changes May 27, 2021

View reviewed changes

vitek-karas mentioned this pull request May 27, 2021

Fix GetLoadContextForDynamicAssembly GC Stress Fail #53341

Merged

vitek-karas merged commit c6ef760 into dotnet:main May 28, 2021

This was referenced May 28, 2021

leakwheel failing in CI #53452

Closed

Host tests often fail on text file busy issues #53587

Closed

ghost locked as resolved and limited conversation to collaborators Jun 27, 2021

Fix #48579 #49387

Fix #48579 #49387

Conversation

Ap0k commented Mar 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunalspathak commented Apr 1, 2021

Ap0k commented Apr 20, 2021

kunalspathak commented Apr 20, 2021

Ap0k commented May 11, 2021

kunalspathak commented May 11, 2021

Ap0k commented May 11, 2021

kunalspathak commented May 11, 2021

Ap0k commented May 11, 2021

kunalspathak commented May 11, 2021

Ap0k commented May 11, 2021

kunalspathak commented May 11, 2021

Ap0k commented May 12, 2021

kunalspathak commented May 13, 2021

Ap0k commented May 13, 2021

Ap0k commented May 13, 2021

kunalspathak commented May 13, 2021

Ap0k commented May 13, 2021

kunalspathak commented May 13, 2021 • edited Loading

Ap0k commented May 24, 2021

kunalspathak commented May 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitek-karas left a comment

Choose a reason for hiding this comment

vitek-karas commented May 27, 2021

AndyAyersMS commented May 27, 2021

vitek-karas commented May 28, 2021

kunalspathak commented May 13, 2021 •

edited

Loading