Refactor ComponentActivator #37784

rseanhall · 2020-06-12T05:43:13Z

Add ComponentActivation test for initialization from self-contained app.
Rename tests around load_assembly_and_get_function_pointer.

Basically addresses PR feedback in #37696 from #37473. Related to #35465.

ghost · 2020-06-12T05:43:18Z

Tagging subscribers to this area: @vitek-karas, @swaroop-sridhar
Notify danmosemsft if you want to be subscribed.

jkotas · 2020-06-12T06:38:28Z

...oreclr/src/System.Private.CoreLib/src/Internal/Runtime/InteropServices/ComponentActivator.cs

@@ -153,5 +128,61 @@ private static IsolatedComponentLoadContext GetIsolatedComponentLoadContext(stri

            return alc;
        }
+
+        private static AssemblyLoadContext InternalLoadAssembly(string assemblyPath)


What's the point of wrapping GetIsolatedComponentLoadContext by another method?

#37696 (comment). It's easy to remove InternalLoadAssembly until we actually need it if we don't want it today.

I would also prefer to not have this method (if we ever introduce a native hosting way to create an ALC, it would have to take the parameter as IntPtr anyway, so I don't really see the usefulness of this wrapper).

jkotas · 2020-06-12T06:43:51Z

...oreclr/src/System.Private.CoreLib/src/Internal/Runtime/InteropServices/ComponentActivator.cs

-                    string delegateTypeName = MarshalToString(delegateTypeNative, nameof(delegateTypeNative));
-                    delegateType = Type.GetType(delegateTypeName, resolver, null, throwOnError: true)!;
-                }
+                (Type? delegateType, string? delegateTypeName) = GetDelegateTypeArgs(delegateTypeNative);


I think the logic is harder to follow now that this is split over 3 different functions. Was it really necessary to split it?

Yes. #37696 (comment)

Can InternalGetFunctionPointer just take delegateTypeNative?

I do not see the point by doing little bit of upfront work in GetDelegateTypeArgs, passing the tuple around, and finishing it in InternalGetFunctionPointer.

The idea is to validate the parameters before doing anything meaningful like creating an ALC. Can't the Marshal call throw an exception if given a bad pointer?

Can't the Marshal call throw an exception if given a bad pointer?

Yes, but it's not really a parameter validation - it will try to read from the pointer, if that throws an AV it will turn into an exception.

I understand the goal here (to validate everything up-front), so I'm basically fine with this, but if @jkotas or others would prefer to not use this, I would also be OK if we duplicate the parameter validation (basically inline the method here as it was before, and then make a copy into the second one we're introducing) - or I would not mind if we simply postpone the marshal call to the point where we're going to actually get the delegate type.

jkotas · 2020-06-12T06:44:38Z

...oreclr/src/System.Private.CoreLib/src/Internal/Runtime/InteropServices/ComponentActivator.cs

@@ -39,6 +39,31 @@ private static string MarshalToString(IntPtr arg, string argName)
            return result;


The two checks for null in MarshalToString is redundant. You can delete one of them while you are on it.

It was probably added so the compiler knows we can't return null. Is adding '!' preferred over this?

! is preferred over adding unreachable code.

vitek-karas · 2020-06-12T07:36:21Z

src/installer/tests/HostActivation.Tests/NativeHosting/ComponentActivation.cs

@@ -211,6 +233,7 @@ public class SharedTestState : SharedTestStateBase
            public string DotNetRoot { get; }

            public TestProjectFixture ComponentWithNoDependenciesFixture { get; }
+            public TestProjectFixture SelfContainedComponentWithNoDependenciesFixture { get; }


Let's not use "self-contained component" since we don't really support that.
Also we're using SDK behavior which is "unsupported" and probably relatively flaky - building classlib with self-contained true.

I would just create a new test application (if we can't use the existing ones), build it as self-contained and use that - the test validates we can load the app as an app and then get a function pointer from it.

I want to avoid confusing in the future when somebody reads the tests and they look like we support self-contained components, when we really don't.

I'm sorry, but I'm going to have to push back on this. There's nothing flaky about using --self-contained with EnableDynamicLoading=True. It works.

"Works on a sample project" != "works always". And even if it did, it's a scenario which is missing several important things:

Nobody actually went through the SDK and made sure that it "should" work

There's no tests for it in the SDK or the runtime

There are cases where it doesn't work outside of SDK (loading such component via AssemblyDependencyResolver will do really weird and "bad" things)

There's no documentation that it should work and if so how

We don't support it (which is not just about it working, but about us signing up to make sure that it's going to continue to work in future releases, fixing bugs in the existing ones and so on)

The reason I don't want this in the tests is that by having a test for it, it's basically saying "we want to support this". But doing that would require lot more work, which we're not planning to do now.

I will change the project that this test uses. I will also create an issue in dotnet/sdk for official support on --self-contained combined with EnableDynamicLoading=True. I would like to point out that this is not necessarily a component. This would be the supported way to build this hybrid app/component.

I don't understand why the runtime needs to add support for this combination. You need to write your own native host to start the runtime for this, but that's the whole point. The only difference between publishing with the currently supported way of publishing self-contained with an executable and this "new" way of EnableDynamicLoading=True will be the presence of the .exe file. The hybrid's dll still needs to be there just like the .exe way, and the .runtimeconfig.json and the .deps.json will (need to) match the .exe way. AssemblyDependencyResolver will have just as much problems with a project that was published the .exe way.

The main reason I'm trying to make a clear distinction between app and component is that that's how SDK and docs and most users see it: app versus classlib. I do agree with all your points, I don't think the problem here is that things won't work in the hybrid scenario (since as you pointed out, it sort of doesn't matter if it's app or classlib). I just think that we need to get the terminology right - so that it leads people to solutions which are likely to work even in other scenarios. Currently if I build something as classlib all our docs and terminology lead me to believe that I can use it as both native (native hosting APIs) and managed (AssemblyDependencyResolver) plugin - if we start officially talking about self-contained components, then I would naturally think that it also works in all these cases. So it's mostly about the right perception. I've seen it too many times that sloppy naming or too vague guidance leads people to solutions which work as a demo, but then break for real apps on some more complex cases. I think it's better if users find the proven mainstream solutions first and only venture into the more complicated ones when absolutely necessary.

So from that point of view calling AsssemblyDependencyResolver on an app is currently simply not supported - we never mention it anywhere. And while it might work in some cases, we don't want people to do it, since they may get into problems.

I would like to point out that this is not necessarily a component. This would be the supported way to build this hybrid app/component.

The problem is that SDK currently only knows about apps or "components" - nothing in between. So the support for --self-contained on classlib projects is essentially support for self-contained components. You may not end up using it as a pure component, but from SDKs point of view it has to look like one. Alternatively we would have to introduce a new concept - some "hybrid" thing.

I'm not trying to shut off special cases - I'm happy if we can make the product work in new interesting ways, but there has to be a distinction between "The recommended proven way of doing things" and "Going off the rails a little bit into this expert level area". I know it sounds like playing with words, but experience tells me it's important.

Yes, sorry, I was lazy with my naming in the tests and it needed to change.

I'm not sure why we're continuing the SDK discussion here instead of in the issue? Like I've said before this is not a component and not an app. We need to stop trying to pretending that it's one or other, it's a new concept.

vitek-karas · 2020-06-12T07:38:04Z

...oreclr/src/System.Private.CoreLib/src/Internal/Runtime/InteropServices/ComponentActivator.cs

@@ -153,5 +128,61 @@ private static IsolatedComponentLoadContext GetIsolatedComponentLoadContext(stri

            return alc;
        }
+
+        private static AssemblyLoadContext InternalLoadAssembly(string assemblyPath)


I would also prefer to not have this method (if we ever introduce a native hosting way to create an ALC, it would have to take the parameter as IntPtr anyway, so I don't really see the usefulness of this wrapper).

vitek-karas · 2020-06-12T07:42:43Z

...oreclr/src/System.Private.CoreLib/src/Internal/Runtime/InteropServices/ComponentActivator.cs

-                    string delegateTypeName = MarshalToString(delegateTypeNative, nameof(delegateTypeNative));
-                    delegateType = Type.GetType(delegateTypeName, resolver, null, throwOnError: true)!;
-                }
+                (Type? delegateType, string? delegateTypeName) = GetDelegateTypeArgs(delegateTypeNative);


Can't the Marshal call throw an exception if given a bad pointer?

Yes, but it's not really a parameter validation - it will try to read from the pointer, if that throws an AV it will turn into an exception.

I understand the goal here (to validate everything up-front), so I'm basically fine with this, but if @jkotas or others would prefer to not use this, I would also be OK if we duplicate the parameter validation (basically inline the method here as it was before, and then make a copy into the second one we're introducing) - or I would not mind if we simply postpone the marshal call to the point where we're going to actually get the delegate type.

Address PR feedback.

...oreclr/src/System.Private.CoreLib/src/Internal/Runtime/InteropServices/ComponentActivator.cs

jkotas · 2020-06-12T15:56:48Z

...oreclr/src/System.Private.CoreLib/src/Internal/Runtime/InteropServices/ComponentActivator.cs

-            {
-                throw new ArgumentNullException(argName);
-            }
-
 #if TARGET_WINDOWS
            string? result = Marshal.PtrToStringUni(arg);
 #else


This can also just call Marshal.PtrToStringAuto and avoid the ifdef.

Interesting, this isn't documented. I can do this. Where should I file an issue for the missing documentation?

https://github.com/dotnet/docs/ is the place to open doc issues

@jkoritzinsky dotnet/coreclr#23664 mentioned that the documentation should have been updated. I guess it fell through the cracks.

AaronRobinsonMSFT · 2020-06-13T00:37:27Z

...oreclr/src/System.Private.CoreLib/src/Internal/Runtime/InteropServices/ComponentActivator.cs

+                    throw new MissingMethodException(typeName, methodName);
+
+                // Verify the function is properly marked.
+                if (null == methodInfo.GetCustomAttribute<UnmanagedCallersOnlyAttribute>())


I think this can be changed to methodInfo.IsDefined(typeof(UnmanagedCallersOnlyAttribute), false). If memory serves @jkotas indicated this was a cheaper call at one point and when I originally wrote this I didn't know that.

I don't want to make this change in this PR. The equivalent code is methodInfo.IsDefined(typeof(UnmanagedCallersOnlyAttribute), true).

jkotas · 2020-06-13T03:50:37Z

@rseanhall Thank you!

rseanhall added 3 commits June 12, 2020 15:37

Refactor ComponentActivator.

3b9480a

Add ComponentActivation test for initialization from self-contained app.

9d61485

Rename tests around load_assembly_and_get_function_pointer.

3f51570

Dotnet-GitSync-Bot added the area-Host label Jun 12, 2020

rseanhall mentioned this pull request Jun 12, 2020

Implement hdt_get_function_pointer #37696

Merged

jkotas reviewed Jun 12, 2020

View reviewed changes

vitek-karas reviewed Jun 12, 2020

View reviewed changes

vitek-karas requested a review from AaronRobinsonMSFT June 12, 2020 07:43

Update app ComponentActivation tests to initialize from an app.

830cfe6

Address PR feedback.

vitek-karas approved these changes Jun 12, 2020

View reviewed changes

swaroop-sridhar approved these changes Jun 12, 2020

View reviewed changes

jkotas reviewed Jun 12, 2020

View reviewed changes

...oreclr/src/System.Private.CoreLib/src/Internal/Runtime/InteropServices/ComponentActivator.cs Show resolved Hide resolved

jkotas reviewed Jun 12, 2020

View reviewed changes

Rely on PtrToStringAuto to call Uni or UTF8

e4da064

AaronRobinsonMSFT approved these changes Jun 13, 2020

View reviewed changes

jkotas merged commit e393cde into dotnet:master Jun 13, 2020

vitek-karas mentioned this pull request Jun 15, 2020

Officially support --self-contained with EnableDynamicLoading=true dotnet/sdk#12043

Open

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ComponentActivator #37784

Refactor ComponentActivator #37784

rseanhall commented Jun 12, 2020

ghost commented Jun 12, 2020

jkotas Jun 12, 2020

rseanhall Jun 12, 2020

vitek-karas Jun 12, 2020

jkotas Jun 12, 2020

rseanhall Jun 12, 2020

jkotas Jun 12, 2020

rseanhall Jun 12, 2020

vitek-karas Jun 12, 2020

jkotas Jun 12, 2020 •

edited

Loading

rseanhall Jun 12, 2020

jkotas Jun 12, 2020

vitek-karas Jun 12, 2020

rseanhall Jun 12, 2020

vitek-karas Jun 12, 2020

rseanhall Jun 12, 2020

vitek-karas Jun 12, 2020

rseanhall Jun 12, 2020

vitek-karas Jun 12, 2020

vitek-karas Jun 12, 2020

jkotas Jun 12, 2020

rseanhall Jun 12, 2020

jkotas Jun 13, 2020

AaronRobinsonMSFT Jun 13, 2020

rseanhall Jun 13, 2020

jkotas commented Jun 13, 2020

		@@ -39,6 +39,31 @@ private static string MarshalToString(IntPtr arg, string argName)
		return result;

Refactor ComponentActivator #37784

Refactor ComponentActivator #37784

Conversation

rseanhall commented Jun 12, 2020

ghost commented Jun 12, 2020

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

jkotas Jun 12, 2020 • edited Loading

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

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

jkotas commented Jun 13, 2020

jkotas Jun 12, 2020 •

edited

Loading