Merge pull request #1247 from microsoft/dev/andarno/fixDynamicAssemblies

Create dynamic proxies in the same ALC as the interface they implement

Author: AArnott

StreamJsonRpc.sln

@@ -1,7 +1,7 @@
 
 Microsoft Visual Studio Solution File, Format Version 12.00
-# Visual Studio Version 17
-VisualStudioVersion = 17.0.31707.426
+# Visual Studio Version 18
+VisualStudioVersion = 18.0.10912.84 main
 MinimumVisualStudioVersion = 10.0.40219.1
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "StreamJsonRpc", "src\StreamJsonRpc\StreamJsonRpc.csproj", "{DFBD1BCA-EAE0-4454-9E97-FA9BD9A0F03A}"
 EndProject
@@ -38,6 +38,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "test", "test", "{F446B894-5
 		test\Directory.Build.targets = test\Directory.Build.targets
 	EndProjectSection
 EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "UnreachableAssembly", "test\UnreachableAssembly\UnreachableAssembly.csproj", "{5AAF7DDA-6CC0-456B-A7E1-B33893915662}"
+EndProject
 Global
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution
 		Debug|Any CPU = Debug|Any CPU
@@ -60,6 +62,10 @@ Global
 		{5936CF62-A59D-4E5C-9EE4-5E8BAFA9F215}.Debug|Any CPU.Build.0 = Debug|Any CPU
 		{5936CF62-A59D-4E5C-9EE4-5E8BAFA9F215}.Release|Any CPU.ActiveCfg = Release|Any CPU
 		{5936CF62-A59D-4E5C-9EE4-5E8BAFA9F215}.Release|Any CPU.Build.0 = Release|Any CPU
+		{5AAF7DDA-6CC0-456B-A7E1-B33893915662}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
+		{5AAF7DDA-6CC0-456B-A7E1-B33893915662}.Debug|Any CPU.Build.0 = Debug|Any CPU
+		{5AAF7DDA-6CC0-456B-A7E1-B33893915662}.Release|Any CPU.ActiveCfg = Release|Any CPU
+		{5AAF7DDA-6CC0-456B-A7E1-B33893915662}.Release|Any CPU.Build.0 = Release|Any CPU
 	EndGlobalSection
 	GlobalSection(SolutionProperties) = preSolution
 		HideSolutionNode = FALSE
@@ -69,6 +75,7 @@ Global
 		{8BF355B2-E3B0-4615-BFC1-7563EADC4F8B} = {F446B894-56AA-4653-ADC0-5FFC911C9C13}
 		{CEF0F77F-19EB-4C76-A050-854984BB0364} = {F446B894-56AA-4653-ADC0-5FFC911C9C13}
 		{5936CF62-A59D-4E5C-9EE4-5E8BAFA9F215} = {F446B894-56AA-4653-ADC0-5FFC911C9C13}
+		{5AAF7DDA-6CC0-456B-A7E1-B33893915662} = {F446B894-56AA-4653-ADC0-5FFC911C9C13}
 	EndGlobalSection
 	GlobalSection(ExtensibilityGlobals) = postSolution
 		SolutionGuid = {4946F7E7-0619-414B-BE56-DDF0261CA8A9}

azure-pipelines/build.yml

@@ -21,7 +21,7 @@ parameters:
 # This is just one of a a few mechanisms to enforce code style consistency.
 - name: EnableDotNetFormatCheck
   type: boolean
-  default: true
+  default: false # disable in v2.22 because it's defective (https://github.com/dotnet/sdk/issues/50262)
 # This lists the names of the artifacts that will be published *from every OS build agent*.
 # Any new tools/artifacts/*.ps1 script needs to be added to this list.
 # If an artifact is only generated or collected on one OS, it should NOT be listed here,

docfx/docs/dynamicproxy.md

@@ -50,3 +50,28 @@ between client and server.
 Sometimes a client may need to block its caller until a response to a JSON-RPC request comes back.
 The dynamic proxy maintains the same async-only contract that is exposed by the @StreamJsonRpc.JsonRpc class itself.
 [Learn more about sending requests](sendrequest.md), particularly under the heading about async responses.
+
+## AssemblyLoadContext considerations
+
+When in a .NET process with multiple <xref:System.Runtime.Loader.AssemblyLoadContext> (ALC) instances, you should consider whether StreamJsonRpc is loaded in an ALC that can load all the types required by the proxy interface.
+
+By default, StreamJsonRpc will generate dynamic proxies in the ALC that the (first) interface requested for the proxy is loaded within.
+This is usually the right choice because the interface should be in an ALC that can resolve all the interface's type references.
+When you request a proxy that implements *multiple* interfaces, and if those interfaces are loaded in different ALCs, you *may* need to control which ALC the proxy is generated in.
+The need to control this may manifest as an <xref:System.MissingMethodException> or <xref:System.InvalidCastException> due to types loading into multiple ALC instances.
+
+In such cases, you may control the ALC used to generate the proxy by surrounding your proxy request with a call to <xref:System.Runtime.Loader.AssemblyLoadContext.EnterContextualReflection*> (and disposal of its result).
+
+For example, you might use the following code when StreamJsonRpc is loaded into a different ALC from your own code:
+
+```cs
+// Whatever ALC can resolve *all* type references in *all* proxy interfaces.
+AssemblyLoadContext alc = AssemblyLoadContext.GetLoadContext(MethodBase.GetCurrentMethod()!.DeclaringType!.Assembly);
+IFoo proxy;
+using (AssemblyLoadContext.EnterContextualReflection(alc))
+{
+    proxy = (IFoo)jsonRpc.Attach([typeof(IFoo), typeof(IFoo2)]);
+}
+```
+
+This initializes the `proxy` local variable with a proxy that will be able to load all types that your own <xref:System.Runtime.Loader.AssemblyLoadContext> can load.

src/StreamJsonRpc/AssemblyNameEqualityComparer.cs

@@ -0,0 +1,36 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+
+using System.Reflection;
+
+namespace StreamJsonRpc;
+
+internal class AssemblyNameEqualityComparer : IEqualityComparer<AssemblyName>
+{
+    internal static readonly IEqualityComparer<AssemblyName> Instance = new AssemblyNameEqualityComparer();
+
+    private AssemblyNameEqualityComparer()
+    {
+    }
+
+    public bool Equals(AssemblyName? x, AssemblyName? y)
+    {
+        if (x is null && y is null)
+        {
+            return true;
+        }
+
+        if (x is null || y is null)
+        {
+            return false;
+        }
+
+        return string.Equals(x.FullName, y.FullName, StringComparison.OrdinalIgnoreCase);
+    }
+
+    public int GetHashCode(AssemblyName obj)
+    {
+        Requires.NotNull(obj, nameof(obj));
+
+        return StringComparer.OrdinalIgnoreCase.GetHashCode(obj.FullName);
+    }
+}

src/StreamJsonRpc/ProxyGeneration.cs

@@ -5,6 +5,9 @@
 using System.Globalization;
 using System.Reflection;
 using System.Reflection.Emit;
+#if NET
+using System.Runtime.Loader;
+#endif
 using Microsoft.VisualStudio.Threading;
 using StreamJsonRpc.Reflection;
 using CodeGenHelpers = StreamJsonRpc.Reflection.CodeGenHelpers;
@@ -18,7 +21,11 @@ namespace StreamJsonRpc;
 
 internal static class ProxyGeneration
 {
+#if NET
+    private static readonly List<(AssemblyLoadContext, ImmutableHashSet<AssemblyName> SkipVisibilitySet, ModuleBuilder Builder)> TransparentProxyModuleBuilderByVisibilityCheck = [];
+#else
     private static readonly List<(ImmutableHashSet<AssemblyName> SkipVisibilitySet, ModuleBuilder Builder)> TransparentProxyModuleBuilderByVisibilityCheck = new List<(ImmutableHashSet<AssemblyName>, ModuleBuilder)>();
+#endif
     private static readonly object BuilderLock = new object();
     private static readonly AssemblyName ProxyAssemblyName = new AssemblyName(string.Format(CultureInfo.InvariantCulture, "StreamJsonRpc_Proxies_{0}", Guid.NewGuid()));
     private static readonly MethodInfo DelegateCombineMethod = typeof(Delegate).GetRuntimeMethod(nameof(Delegate.Combine), new Type[] { typeof(Delegate), typeof(Delegate) })!;
@@ -104,6 +111,9 @@ internal static TypeInfo Get(Type contractInterface, ReadOnlySpan<Type> addition
             // Rpc interfaces must be sorted so that we implement methods from base interfaces before those from their derivations.
             SortRpcInterfaces(rpcInterfaces);
 
+            // For ALC selection reasons, it's vital that the *user's* selected interfaces come *before* our own supporting interfaces.
+            // If the order is incorrect, type resolution may fail or the wrong AssemblyLoadContext (ALC) may be selected,
+            // leading to runtime errors or unexpected behavior when loading types or invoking methods.
             Type[] proxyInterfaces = [.. rpcInterfaces.Select(i => i.Type), typeof(IJsonRpcClientProxy), typeof(IJsonRpcClientProxyInternal)];
             ModuleBuilder proxyModuleBuilder = GetProxyModuleBuilder(proxyInterfaces);
 
@@ -752,10 +762,27 @@ private static ModuleBuilder GetProxyModuleBuilder(Type[] interfaceTypes)
         // For each set of skip visibility check assemblies, we need a dynamic assembly that skips at *least* that set.
         // The CLR will not honor any additions to that set once the first generated type is closed.
         // We maintain a dictionary to point at dynamic modules based on the set of skip visibility check assemblies they were generated with.
-        ImmutableHashSet<AssemblyName> skipVisibilityCheckAssemblies = ImmutableHashSet.CreateRange(interfaceTypes.SelectMany(t => SkipClrVisibilityChecks.GetSkipVisibilityChecksRequirements(t.GetTypeInfo())))
+        ImmutableHashSet<AssemblyName> skipVisibilityCheckAssemblies = ImmutableHashSet.CreateRange(AssemblyNameEqualityComparer.Instance, interfaceTypes.SelectMany(t => SkipClrVisibilityChecks.GetSkipVisibilityChecksRequirements(t.GetTypeInfo())))
             .Add(typeof(ProxyGeneration).Assembly.GetName());
+#if NET
+        // We have to key the dynamic assembly by ALC as well, since callers may set a custom contextual reflection context
+        // that influences how the assembly will resolve its type references.
+        // If they haven't set a contextual one, we assume the ALC that defines the (first) proxy interface.
+        AssemblyLoadContext alc = AssemblyLoadContext.CurrentContextualReflectionContext
+            ?? AssemblyLoadContext.GetLoadContext(interfaceTypes[0].Assembly)
+            ?? AssemblyLoadContext.GetLoadContext(typeof(ProxyGeneration).Assembly)
+            ?? throw new Exception("No ALC for our own assembly!");
+        foreach ((AssemblyLoadContext AssemblyLoadContext, ImmutableHashSet<AssemblyName> SkipVisibilitySet, ModuleBuilder Builder) existingSet in TransparentProxyModuleBuilderByVisibilityCheck)
+        {
+            if (existingSet.AssemblyLoadContext != alc)
+            {
+                continue;
+            }
+
+#else
         foreach ((ImmutableHashSet<AssemblyName> SkipVisibilitySet, ModuleBuilder Builder) existingSet in TransparentProxyModuleBuilderByVisibilityCheck)
         {
+#endif
             if (existingSet.SkipVisibilitySet.IsSupersetOf(skipVisibilityCheckAssemblies))
             {
                 return existingSet.Builder;
@@ -767,11 +794,22 @@ private static ModuleBuilder GetProxyModuleBuilder(Type[] interfaceTypes)
         // I have disabled this optimization though till we need it since it would sometimes cover up any bugs in the above visibility checking code.
         ////skipVisibilityCheckAssemblies = skipVisibilityCheckAssemblies.Union(AppDomain.CurrentDomain.GetAssemblies().Select(a => a.GetName()));
 
-        AssemblyBuilder assemblyBuilder = CreateProxyAssemblyBuilder();
+        AssemblyBuilder assemblyBuilder;
+#if NET
+        using (alc.EnterContextualReflection())
+#endif
+        {
+            assemblyBuilder = CreateProxyAssemblyBuilder();
+        }
+
         ModuleBuilder moduleBuilder = assemblyBuilder.DefineDynamicModule("rpcProxies");
         var skipClrVisibilityChecks = new SkipClrVisibilityChecks(assemblyBuilder, moduleBuilder);
         skipClrVisibilityChecks.SkipVisibilityChecksFor(skipVisibilityCheckAssemblies);
+#if NET
+        TransparentProxyModuleBuilderByVisibilityCheck.Add((alc, skipVisibilityCheckAssemblies, moduleBuilder));
+#else
         TransparentProxyModuleBuilderByVisibilityCheck.Add((skipVisibilityCheckAssemblies, moduleBuilder));
+#endif
 
         return moduleBuilder;
     }

src/StreamJsonRpc/SkipClrVisibilityChecks.cs

@@ -79,7 +79,7 @@ internal static ImmutableHashSet<AssemblyName> GetSkipVisibilityChecksRequiremen
         Requires.NotNull(typeInfo, nameof(typeInfo));
 
         var visitedTypes = new HashSet<TypeInfo>();
-        ImmutableHashSet<AssemblyName>.Builder assembliesDeclaringInternalTypes = ImmutableHashSet.CreateBuilder<AssemblyName>(AssemblyNameEqualityComparer.Instance);
+        ImmutableHashSet<AssemblyName>.Builder assembliesDeclaringInternalTypes = ImmutableHashSet.CreateBuilder(AssemblyNameEqualityComparer.Instance);
         CheckForNonPublicTypes(typeInfo, assembliesDeclaringInternalTypes, visitedTypes);
 
         // Enumerate members on the interface that we're going to need to implement.
@@ -253,35 +253,4 @@ private TypeInfo EmitMagicAttribute()
 
         return tb.CreateTypeInfo()!;
     }
-
-    private class AssemblyNameEqualityComparer : IEqualityComparer<AssemblyName>
-    {
-        internal static readonly IEqualityComparer<AssemblyName> Instance = new AssemblyNameEqualityComparer();
-
-        private AssemblyNameEqualityComparer()
-        {
-        }
-
-        public bool Equals(AssemblyName? x, AssemblyName? y)
-        {
-            if (x is null && y is null)
-            {
-                return true;
-            }
-
-            if (x is null || y is null)
-            {
-                return false;
-            }
-
-            return string.Equals(x.FullName, y.FullName, StringComparison.OrdinalIgnoreCase);
-        }
-
-        public int GetHashCode(AssemblyName obj)
-        {
-            Requires.NotNull(obj, nameof(obj));
-
-            return StringComparer.OrdinalIgnoreCase.GetHashCode(obj.FullName);
-        }
-    }
 }

test/StreamJsonRpc.Tests/JsonRpcProxyGenerationTests.cs

@@ -1,8 +1,13 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+#if NET
+using System.Reflection;
+using System.Runtime.Loader;
+#endif
 using Microsoft.VisualStudio.Threading;
 using Nerdbank;
+using StreamJsonRpc.Tests;
 using ExAssembly = StreamJsonRpc.Tests.ExternalAssembly;
 
 public class JsonRpcProxyGenerationTests : TestBase
@@ -135,6 +140,11 @@ public interface IServerWithGenericMethod
         Task AddAsync<T>(T a, T b);
     }
 
+    public interface IReferenceAnUnreachableAssembly
+    {
+        Task TakeAsync(UnreachableAssembly.SomeUnreachableClass obj);
+    }
+
     internal interface IServerInternal :
         ExAssembly.ISomeInternalProxyInterface,
         IServerInternalWithInternalTypesFromOtherAssemblies,
@@ -798,6 +808,57 @@ public async Task ValueTaskReturningMethod()
         await clientRpc.DoSomethingValueAsync();
     }
 
+    /// <summary>
+    /// Validates that similar proxies are generated in the same dynamic assembly.
+    /// </summary>
+    [Fact]
+    public void ReuseDynamicAssembliesTest()
+    {
+        JsonRpc clientRpc = new(Stream.Null);
+        IServer proxy1 = clientRpc.Attach<IServer>();
+        IServer2 proxy2 = clientRpc.Attach<IServer2>();
+        Assert.Same(proxy1.GetType().Assembly, proxy2.GetType().Assembly);
+    }
+
+#if NET
+    [Fact]
+    public void DynamicAssembliesKeyedByAssemblyLoadContext()
+    {
+        UnreachableAssemblyTools.VerifyUnreachableAssembly();
+
+        // Set up a new ALC that can find the hidden assembly, and ask for the proxy type.
+        AssemblyLoadContext alc = UnreachableAssemblyTools.CreateContextForReachingTheUnreachable();
+
+        JsonRpc clientRpc = new(Stream.Null);
+
+        // Ensure we first generate a proxy in our own default ALC.
+        // The goal being to emit a DynamicAssembly that we *might* reuse
+        // for the later proxy for which the first DynamicAssembly is not appropriate.
+        clientRpc.Attach<IServer>();
+
+        // Now take very specific steps to invoke the rest of the test in the other AssemblyLoadContext.
+        // This is important so that our IReferenceAnUnreachableAssembly type will be able to resolve its
+        // own type references to UnreachableAssembly.dll, which our own default ALC cannot do.
+        MethodInfo helperMethodInfo = typeof(JsonRpcProxyGenerationTests).GetMethod(nameof(DynamicAssembliesKeyedByAssemblyLoadContext_Helper), BindingFlags.NonPublic | BindingFlags.Static)!;
+        MethodInfo helperWithinAlc = UnreachableAssemblyTools.LoadHelperInAlc(alc, helperMethodInfo);
+        helperWithinAlc.Invoke(null, null);
+    }
+
+    private static void DynamicAssembliesKeyedByAssemblyLoadContext_Helper()
+    {
+        // Although this method executes within the special ALC,
+        // StreamJsonRpc is loaded in the default ALC.
+        // Therefore unless StreamJsonRpc is taking care to use a DynamicAssembly
+        // that belongs to *this* ALC, it won't be able to resolve the same type references
+        // that we can here (the ones from UnreachableAssembly).
+        // That's what makes this test effective: it'll fail if the DynamicAssembly is shared across ALCs,
+        // thereby verifying that StreamJsonRpc has a dedicated set of DynamicAssemblies for each ALC.
+        JsonRpc clientRpc = new(Stream.Null);
+        clientRpc.Attach<IReferenceAnUnreachableAssembly>();
+    }
+
+#endif
+
     public class EmptyClass
     {
     }

test/StreamJsonRpc.Tests/StreamJsonRpc.Tests.csproj

@@ -47,6 +47,10 @@
   </ItemGroup>
   <ItemGroup>
     <ProjectReference Include="..\StreamJsonRpc.Tests.ExternalAssembly\StreamJsonRpc.Tests.ExternalAssembly.csproj" />
+    <ProjectReference Include="..\UnreachableAssembly\UnreachableAssembly.csproj">
+      <!-- We MUST NOT have this assembly in our test assembly's output directory so that it cannot be reached from the default ALC. -->
+      <Private>false</Private>
+    </ProjectReference>
     <ProjectReference Include="..\..\src\StreamJsonRpc\StreamJsonRpc.csproj" />
   </ItemGroup>
   <ItemGroup>
@@ -68,4 +72,10 @@
   <ItemGroup>
     <Reference Include="Microsoft.CSharp" Condition=" '$(TargetFramework)' == 'net472' " />
   </ItemGroup>
+  <Target Name="PlaceMissingAssembly" AfterTargets="ResolveReferences">
+    <ItemGroup>
+      <DivertedProjectReferenceOutputs Include="@(ReferencePath)" Condition=" '%(FileName)' == 'UnreachableAssembly' " />
+    </ItemGroup>
+    <Copy SourceFiles="@(DivertedProjectReferenceOutputs)" DestinationFolder="$(OutputPath)hidden" SkipUnchangedFiles="true" />
+  </Target>
 </Project>

test/StreamJsonRpc.Tests/UnreachableAssemblyTools.cs

@@ -0,0 +1,50 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+#if NET
+
+using System.Reflection;
+using System.Runtime.CompilerServices;
+using System.Runtime.Loader;
+
+namespace StreamJsonRpc.Tests;
+
+internal static class UnreachableAssemblyTools
+{
+    /// <summary>
+    /// Useful for tests to call before asserting conditions that depend on the UnreachableAssembly.dll
+    /// actually being unreachable.
+    /// </summary>
+    internal static void VerifyUnreachableAssembly()
+    {
+        Assert.Throws<FileNotFoundException>(() => typeof(UnreachableAssembly.SomeUnreachableClass));
+    }
+
+    /// <summary>
+    /// Initializes an <see cref="AssemblyLoadContext"/> with UnreachableAssembly.dll loaded into it.
+    /// </summary>
+    /// <param name="testName">The name to give the <see cref="AssemblyLoadContext"/>.</param>
+    /// <returns>The new <see cref="AssemblyLoadContext"/>.</returns>
+    internal static AssemblyLoadContext CreateContextForReachingTheUnreachable([CallerMemberName] string? testName = null)
+    {
+        AssemblyLoadContext alc = new(testName);
+        alc.LoadFromAssemblyPath(Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location)!, "hidden", "UnreachableAssembly.dll"));
+        return alc;
+    }
+
+    /// <summary>
+    /// Translates a <see cref="MethodInfo"/> from one ALC into another ALC, so that it can be invoked
+    /// within the context of the new ALC.
+    /// </summary>
+    /// <param name="alc">The <see cref="AssemblyLoadContext"/> to load the method into.</param>
+    /// <param name="helperMethodInfo">The <see cref="MethodInfo"/> of the method in the caller's ALC to load into the given <paramref name="alc"/>.</param>
+    /// <returns>The translated <see cref="MethodInfo"/>.</returns>
+    internal static MethodInfo LoadHelperInAlc(AssemblyLoadContext alc, MethodInfo helperMethodInfo)
+    {
+        Assembly selfWithinAlc = alc.LoadFromAssemblyPath(helperMethodInfo.DeclaringType!.Assembly.Location);
+        MethodInfo helperWithinAlc = (MethodInfo)selfWithinAlc.ManifestModule.ResolveMethod(helperMethodInfo.MetadataToken)!;
+        return helperWithinAlc;
+    }
+}
+
+#endif