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

#98551 - Regression in DI scope validation #98661

Merged
merged 4 commits into from
Feb 27, 2024

Conversation

christiaanderidder
Copy link
Contributor

Fixes #98551

@ghost
Copy link

ghost commented Feb 19, 2024

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

Issue Details

Fixes #98551

Author: christiaanderidder
Assignees: -
Labels:

area-Extensions-DependencyInjection

Milestone: -

// Store the result for each visited service
_scopedServices[callSite.Cache.Key] = scoped;
// If there is a scoped service in the call site tree, make sure we are not resolving it from a singleton
if (firstScopedServiceInCallSiteTree != null && argument.Singleton != null)
Copy link
Member

Choose a reason for hiding this comment

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

serviceCollection.AddSingleton<IFoo, Foo>();

// Act + Assert
var aggregateException = Assert.Throws<AggregateException>(() => serviceCollection.BuildServiceProvider(new ServiceProviderOptions() { ValidateOnBuild = true, ValidateScopes = true }));
Copy link
Member

Choose a reason for hiding this comment

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

I assume without the fix here, that no exception was thrown here.

Copy link
Contributor Author

@christiaanderidder christiaanderidder Feb 22, 2024

Choose a reason for hiding this comment

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

An exception was indeed not thrown here, however it was thrown when registering the dependencies in the reversed order. The aspnet core repo happens to test the this scenario, while the runtime repo only covered the latter. This happened because in the previous code, the scoped-in-singleton check was not performed if a callsite was already cached. See here as well: #98551 (comment)

Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

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

LGTM; waiting a bit for others to comment.

@carlossanlop
Copy link
Member

We need this backported to release/9.0-preview2, it's blocking the build.

@steveharter @benjaminpetit can you please validate the CI failure?

@carlossanlop
Copy link
Member

The CI failure seems pre-existing: #98406

It's been signed-off by area owners so I'll go ahead and merge, then backport.

@hoyosjs
Copy link
Member

hoyosjs commented Feb 27, 2024

@carlossanlop that's #98406. Build analysis is green.

@carlossanlop
Copy link
Member

LGTM; waiting a bit for others to comment.

@steveharter please help keep an eye on this later.

@carlossanlop carlossanlop merged commit 8e59df8 into dotnet:main Feb 27, 2024
109 of 111 checks passed
@carlossanlop
Copy link
Member

/backport to release/9.0-preview2

Copy link
Contributor

Started backporting to release/9.0-preview2: https://github.com/dotnet/runtime/actions/runs/8069902963

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants