-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 DI regression with FactoryCallSite #57155
Conversation
Tagging subscribers to this area: @eerhardt, @maryamariyan Issue Detailsnull
|
...crosoft.Extensions.DependencyInjection/tests/DI.Tests/DependencyInjectionEventSourceTests.cs
Outdated
Show resolved
Hide resolved
public class ServiceCollectionDescriptorExtensionsTest | ||
{ | ||
[Fact] | ||
internal void GetService_FactoryCallSite_Transient_DoesNotFail() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this test catch the bug you are fixing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain how? Why is DependencyInjectionEventSource.Log.ServiceRealizationFailed fired? I thought we just put the compiled accessor into the wrong slot and don't use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the transient service is being registered as a factory callsite, then ImplementationType will be null and will cause ArgumentNullException on
_realizedServices[callSite.ImplementationType] = accessor;
and cause Debug.Fail and that log to get fired.
cc @eerhardt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FactoryCallSite has a null implementation type so it throws argument null here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a pretty indirect way to test it.
We can test it more directly by registering
IService -> Service
Service -> Service
And ensuring that both accessors return distinct values.
What I don't understand is don't we already have a test that uses a transient factory using a dynamic service provider? Or is the problem that we don't actually get to compile it in those tests?
Should we add a mode that uses the dynamic engine but forces the compilation eagerly and run all tests through it? Otherwise, we would keep adding more variations of existing tests with for and Thread.Sleep in them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I'll check where these similar tests are. I updated the PR description to describe the two scenarios being fixed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test that does the two above registrations with transient lifetime and using factory callsite, kicks off background thread and after the fix Debug.Fail, won't hit anymore.
....Extensions.DependencyInjection/tests/DI.Tests/ServiceCollectionDescriptorExtensionsTests.cs
Outdated
Show resolved
Hide resolved
....Extensions.DependencyInjection/tests/DI.Tests/ServiceCollectionDescriptorExtensionsTests.cs
Outdated
Show resolved
Hide resolved
@eerhardt @davidfowl @pakrym ready for another review. I think all comments are addressed |
40ebbee
to
645483a
Compare
- Use existing mock types
645483a
to
d2cf158
Compare
....Extensions.DependencyInjection/tests/DI.Tests/ServiceCollectionDescriptorExtensionsTests.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceLookup/CallSiteFactoryTest.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceLookup/CallSiteFactoryTest.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceLookup/CallSiteFactoryTest.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks!
Hello @maryamariyan! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Improvements: dotnet/perf-autofiling-issues#607 |
Improvements: dotnet/perf-autofiling-issues#566 |
Improvements in arm64: dotnet/perf-autofiling-issues#620, dotnet/perf-autofiling-issues#627 |
This is fixing regression caused by PR refactor #52140
Helps fix regressions described in: #54170 and #56781
runtime/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs
Line 170 in 7c8f5fb
needs to instead be
We're addressing two bugs:
ArgumentNullException
throwing when ImplementationType isnull
for factory callsites.