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

Microsoft DI resolves enumerables incorrectly when a matching ServiceType has already been resolved #87017

Closed
mesakomarevich opened this issue Jun 1, 2023 · 11 comments · Fixed by #80410

Comments

@mesakomarevich
Copy link

mesakomarevich commented Jun 1, 2023

Description

When a user registers multiple services with matching a serviceType in a ServiceCollection and tries to resolve them with the GetServices<T> extension method, they will receive IEnumerable<T> of the correct length but with some services duplicated, and others missing. This will occur if the following conditions are met:

  • Any number of service(s) T have been previously resolved and cached
  • The cached service(s) T do not have ServiceCacheKey.Slot values that match their inverse order of appearance in the _descriptors array in CallSiteFactory

Reproduction Steps

I found this bug while investigating an issue with MediatR, and followed the (very) long debugging path to CallSiteFactory by using the minimal reproduction provided by @MichaelHochriegl for the MediatR issue here.

Expected behavior

When calling GetServices<T>, all services registered with T as their service type are returned, regardless of which service(s) T have been resolved earlier in the program.

Actual behavior

GetServices<T> returns an erroneous list of services of the expected length, but with some services being duplicated and others missing.

Regression?

I believe that this issue was introduced by the changes made for #52035 as the root cause of the issue is the use of the _callSiteCache to optimize service lookups.

Known Workarounds

None.

Configuration

  • .NET: 6, 7, 8 (preview)
  • OS: Windows 11 22621.1778
  • Architecture: x64
  • This issue is not related to computer or OS configuration, and exists in version 6.0.0 and 7.0.0 of the Microsoft.Extensions.DependencyInjection.Abstractions nuget package.

Other information

I have already written a fix for this issue and will create a PR/draft PR for it once I clean up my branch a bit, but would greatly appreciate any suggestions on the changes made. Feel free to assign this issue to me.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 1, 2023
@ghost
Copy link

ghost commented Jun 1, 2023

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

When a user registers multiple services with matching a serviceType in a ServiceCollection and tries to resolve them with the GetServices<T> extension method, they will receive IEnumerable<T> of the correct length but with some services duplicated, and others missing. This will occur if the following conditions are met:

  • Any number of service(s) T have been previously resolved and cached
  • The cached service(s) T do not have ServiceCacheKey.Slot values that match their inverse order of appearance in the _descriptors array in CallSiteFactory

Reproduction Steps

I found this bug while investigating an issue with MediatR, and followed the (very) long debugging path to CallSiteFactory by using the minimal reproduction provided by @MichaelHochriegl for the MediatR issue here.

Expected behavior

When calling GetServices<T>, all services registered with T as their service type are returned, regardless of which service(s) T have been resolved earlier in the program.

Actual behavior

GetServices<T> returns an erroneous list of services of the expected length, but with some services being duplicated and others missing.

Regression?

I believe that this issue was introduced by the changes made for #52035 as the root cause of the issue is the use of the _callSiteCache to optimize service lookups.

Known Workarounds

None.

Configuration

  • .NET: 6, 7, 8 (preview)
  • OS: Windows 11 22621.1778
  • Architecture: x64
  • This issue is not related to computer or OS configuration, and exists in version 6.0.0 and 7.0.0 of the Microsoft.Extensions.DependencyInjection.Abstractions nuget package.

Other information

I have already written a fix for this issue and will create a PR/draft PR for it once I clean up my branch a bit, but would greatly appreciate any suggestions on the changes made.

Author: mesakomarevich
Assignees: -
Labels:

untriaged, area-Extensions-DependencyInjection

Milestone: -

mesakomarevich added a commit to mesakomarevich/runtime that referenced this issue Jun 1, 2023
We now attempt to match each descriptor against both the service type
and implementation type of cached call sites to account for slot mismatches

Added unit test to validate this functionality

Fix dotnet#87017
@mesakomarevich
Copy link
Author

PR has been created here with a fix.

@cameronboutilier
Copy link

Thank god, this bug was so annoying.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 5, 2023
@steveharter
Copy link
Member

I believe that this issue was introduced by the changes made for #52035 as the root cause of the issue is the use of the _callSiteCache to optimize service lookups.

Thoughts @davidfowl?

@davidfowl
Copy link
Member

davidfowl commented Jun 6, 2023

Yikes, good find!

EDIT: @mesakomarevich can you verify if #80410 fixes your issue?

I'm wondering if this is the same bug.

@mesakomarevich
Copy link
Author

@davidfowl Looking into this now

@mesakomarevich
Copy link
Author

@davidfowl after merging the tests added in each branch and running them against my changes and the changes in #80410, I can confirm that both branches pass all tests, and that #80410 fixes my issue. So it seems that either solution should work.

There may be some performance differences between our changes, but by squinting real hard at the code, I don't imagine that they would be very significant.

@steveharter
Copy link
Member

Are there any requests to backport the fix to v7 and possible v6 (since it's LTS)?

@davidfowl
Copy link
Member

I think we should backport once we're confident in the fix. We may want to wait for this change to exist in .NET 8 for a bit before we do that.

@ghost ghost removed in-pr There is an active PR which will close this issue when it is merged untriaged New issue has not been triaged by the area owner labels Jun 9, 2023
@mesakomarevich
Copy link
Author

@davidfowl could we possibly release a beta version of a backport to v6 and v7? As per this issue raised in MediatR, there are a couple of us who have run into this issue in production code and are looking for a solution.

@davidfowl
Copy link
Member

We don’t do beta versions of patches. We’ll back port the fix for sure. It would be great if you could use the 8.0 preview of this package to help verify it (once it comes out).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.