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

Fix NullReferenceException in background thread emitting IL in Depend… #55340

Merged
merged 3 commits into from
Jul 8, 2021

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Jul 8, 2021

…encyInjection

When ILEmitResolverBuilder is getting created, it grabs the "Root" scope off of the ServiceProvider. However, the Root scope isn't set on ServiceProvider yet. So later when it tries to get used, it null refs. But this exception gets caught an eaten since it happens on a background thread.

The fix is to set Root before creating the ServiceProviderEngine.

See description of the issue in #55255 (comment).

…encyInjection

When ILEmitResolverBuilder is getting created, it grabs the "Root" scope off of the ServiceProvider. However, the Root scope isn't set on ServiceProvider yet. So later when it tries to get used, it null refs. But this exception gets caught an eaten since it happens on a background thread.

The fix is to set Root before creating the ServiceProviderEngine.
@ghost
Copy link

ghost commented Jul 8, 2021

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

…encyInjection

When ILEmitResolverBuilder is getting created, it grabs the "Root" scope off of the ServiceProvider. However, the Root scope isn't set on ServiceProvider yet. So later when it tries to get used, it null refs. But this exception gets caught an eaten since it happens on a background thread.

The fix is to set Root before creating the ServiceProviderEngine.

Author: eerhardt
Assignees: -
Labels:

area-Extensions-DependencyInjection

Milestone: -

@davidfowl
Copy link
Member

I'm betting this it the cause of the performance regression on ARM.

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

One small nit

@pakrym
Copy link
Contributor

pakrym commented Jul 8, 2021

What do you think about adding a Debug.Assert to https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/DynamicServiceProviderEngine.cs#L39 ?

@eerhardt
Copy link
Member Author

eerhardt commented Jul 8, 2021

Installer Build and Test coreclr Linux_arm64 Release test failure is infrastructure related, and unrelated to this change.

@eerhardt eerhardt merged commit 2432926 into dotnet:main Jul 8, 2021
@eerhardt eerhardt deleted the FixDIILEmit branch July 8, 2021 20:29
@ghost ghost locked as resolved and limited conversation to collaborators Aug 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants