Skip to content
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.

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

ProviderTypeLoader: do not enumerate types in ReflectionOnly assembly. #2869

Merged
merged 1 commit into from
Mar 22, 2017

Conversation

ReubenBond
Copy link
Member

Also improve logging for errors in TypeUtils.GetDefinedTypes(...) so that loader exceptions are made apparent.

Intention is to make debugging issues like #2840 more obvious

@@ -87,6 +87,14 @@ private void ProcessAssemblyLocally(Assembly assembly)

private static void ProcessNewAssembly(object sender, AssemblyLoadEventArgs args)
{
#if !NETSTANDARD
// If the assembly is loaded for reflection only avoid processing it.
if (args.LoadedAssembly.ReflectionOnly)
Copy link
Member

Choose a reason for hiding this comment

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

The linked issue doesn't look like it was related to the ProviderTypeLoader. What led you to here?
Also, I think that because this is wired to the AppDomain.AssemblyLoad event, it should not be getting called when a reflection-only assembly is loaded (I might be wrong, because there's too much black magic with this context, but I think that's what I remember).

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought I had replied to this, but apparently not.

This was just me enumerating usages of GetDefinedTypes to see where it was being potentially used incorrectly. AssemblyLoad is called when asms are loaded ReflectionOnly - I tested in a clean project.

Copy link
Member

Choose a reason for hiding this comment

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

OK, and I assume it still works correctly with custom providers that are loaded via reflection, right? We scan for IProvider in a separate step, but by the time it reaches here it should already be fully loaded?

Copy link
Member Author

Choose a reason for hiding this comment

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

When a provider dll is loaded as non-ReflectionOnly, it will call this again with that asm.

Eventually, we need to overhaul AssemblyLoaded and avoid ReflectionOnly

Copy link
Member

Choose a reason for hiding this comment

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

I meant, we would only fully load the assembly once we first did a pass via reflection-only and found that there is at least one type that implements IProvider (unless I'm not remembering correctly). That step is separate from this, or is this the step that actually scans the reflection only assemblies and decides whether to fully load it?

Copy link
Member Author

Choose a reason for hiding this comment

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

The code path which is called hits RegisterProviderType which would fail for a ReflectionOnly type.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, sounds good. I checked and the code I was referring to happens before this is hit, so it's as expected. BTW, I was referring to:

AssemblyLoaderCriteria.LoadTypesAssignableFrom(typeof(IProvider))
and (which first scans assemblies using reflection-only, and if it finds something that implements IProvider, it fully loads them)

@jdom jdom merged commit c739963 into dotnet:master Mar 22, 2017
@ReubenBond ReubenBond deleted the typeutils-getdefinedtypes-log branch March 22, 2017 01:16
jdom pushed a commit to jdom/orleans that referenced this pull request Mar 22, 2017
sergeybykov pushed a commit that referenced this pull request Mar 22, 2017
@github-actions github-actions bot locked and limited conversation to collaborators Dec 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants