Load Assembly referenced from within a lambda closure

[suhsteve]

Please see example in issue #120

The assembly containing the type used in the lambda closure is not loaded as part of the Worker's application domain. When the Worker deserializes the object within the lambda, the type must be loaded from an external Assembly.

This issue seems to be isolated to dotnet core. Using .NET framework, the assembly automatically loaded.

[eerhardt]

Have you checked out https://docs.microsoft.com/en-us/dotnet/framework/deployment/best-practices-for-assembly-loading? I don't think Assembly.LoadFrom is recommended anymore. But instead, using AssemblyLoadContext.

/cc @stephentoub

[eerhardt]

See also the way MSBuild loads assemblies for an example:

https://github.com/microsoft/msbuild/blob/fc10ea8ce260b764bb9fa5033b327af9fefcaabe/src/Shared/TaskEngineAssemblyResolver.cs#L127

[suhsteve]

Thanks for the suggestion and finding the article. For now we will create issue #138 and revisit the assembly loading in another PR.

[suhsteve]

@eerhardt I investigated this a bit more. The AssemblyLoadContext relies is only available for netcore and not net framework. Ideally we would want to do something like the msbuild example, using #if preprocessor directives and choosing to use Assembly.LoadFrom or AssemblyLoadContext depending on the framework. However since Microsoft.Spark library is targeting netstandard2.0 , i'm not sure how to go about doing this. Would you have any suggestions on how we can do this ?

[eerhardt]

I think there are 2 possibilities:

1. Detect at runtime if we are running on .NET Framework - which you can use https://source.dot.net/#CoreFx.Private.TestUtilities/System/PlatformDetection.cs,24.
2. Multi-target Microsoft.Spark to build for both net45 and netcoreapp2.1. But we would still need to target netstandard2.0 as well so other netstandard libraries could reference Spark. This would involve more work and build time, since we would be compiling the assembly 3 times.

My suggestion would be to try (1) above, and fallback to (2) if necessary.

[suhsteve]

@eerhardt thanks for your suggestions.

I've tried (1) and while it compiles, it fails at runtime, where it tries to load the incompatible library. I suspect that the net461 Microsoft.Spark.Worker attempts to load it because Microsoft.Spark.dll still contains references to the library.

(2) worked, but we run into the concerns you brought up regarding building separate libraries targeting multiple frameworks. What do you think about the following approach?

Add an AssemblyLoader.cs class to Microsoft.Spark (netstandard2.0) that looks like the following:

    internal static class AssemblyLoader
    {
        internal static Func<string, Assembly> Loader { get; set; } = Assembly.Load;

        internal static Assembly LoadAssembly(string manifestModuleName)
        {
            var sep = Path.DirectorySeparatorChar;
            try
            {
                return Loader(
                    $"{Directory.GetCurrentDirectory()}{sep}{manifestModuleName}");
            }
            catch (FileNotFoundException)
            {
                return Loader(
                    $"{AppDomain.CurrentDomain.BaseDirectory}{sep}{manifestModuleName}");
            }
        }

In Microsoft.Spark.Worker, which targets (net461 and netcoreapp2.1), we can add #if preprocessor directives and set the above AssemblyLoader.Loader property to use AssemblyLoadContext if the target framework is netcore.

[suhsteve]

I am loading the assembly by way of the delegate mentioned previously. This seemed like the cleanest approach.

[suhsteve]

This seems to be related to https://github.com/dotnet/coreclr/issues/11498 . Will explore more when addressing #138 .

[imback82]

@suhsteve what will be the effort to do #138? Since both this PR and #138 are breaking changes, it will be nice to have this in one release.

[imback82]

@suhsteve what will be the effort to do #138? Since both this PR and #138 are breaking changes, it will be nice to have this in one release.

Let's release this one first and come back to #138.

[imback82]

I think ValueData is always non-null. Should we either assert or field.ValueData.Value? Otherwise, we will get null exception down the stream, so it may be a bit confusing.

[suhsteve]

ValueData can potentially be null. I updated the UdfSerDeTests with an example.

[imback82]

Which test is covering this? Can ValueData be null in this context? Note that it's a delegate in this context (not capture). Can you check comments in line 70 in this file?

[suhsteve]

Oh you're right. My tests do not touch CommandSerde.cs . Looks like it only gets to this case when the current node is a non-UDF, a MapUdfWrapper or WorkerChainHelper. I'll revert this change.

[imback82]

LGTM