Skip to content

Fix leak caused by not disposing the scoped parent service provider - #74261

Merged
tarekgh merged 2 commits into
dotnet:mainfrom
tarekgh:FixDisposingScopedParentServiceProvider
Aug 22, 2022
Merged

Fix leak caused by not disposing the scoped parent service provider#74261
tarekgh merged 2 commits into
dotnet:mainfrom
tarekgh:FixDisposingScopedParentServiceProvider

Conversation

@tarekgh

@tarekghtarekgh commented Aug 19, 2022

Copy link
Copy Markdown
Member

Fixes#73832

@ghostghost assigned tarekghAug 19, 2022
@ghost

ghost commented Aug 19, 2022

Copy link
Copy Markdown

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

Issue Details

Fixes #73832

Author:tarekgh
Assignees:-
Labels:

area-Extensions-DependencyInjection

Milestone:-

@tarekghtarekgh changed the title Fix leaks caused by not disposing the parent scoped ServiceProviderFix leak caused by not disposing the parent scoped ServiceProviderAug 19, 2022
@tarekghtarekgh added this to the 7.0.0 milestone Aug 19, 2022
@tarekgh

tarekgh commented Aug 19, 2022

Copy link
Copy Markdown
MemberAuthor

CC @ericstj

@tarekghtarekgh changed the title Fix leak caused by not disposing the parent scoped ServiceProviderFix leak caused by not disposing the scoped parent service providerAug 19, 2022
eerhardt
eerhardt reviewed Aug 19, 2022
View reviewed changes
Comment thread...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs
Comment thread...s/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs Outdated
Comment thread...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs
@runfoapprunfoappBot mentioned this pull request Aug 19, 2022
Closed
davidfowl
davidfowl reviewed Aug 20, 2022
View reviewed changes
Comment thread...ies/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs
// If this ServiceProviderEngineScope instance is a root scope, disposing this instance will need to dispose the RootProvider too.
// Otherwise the RootProvider will never get disposed and will leak.
// Note, if the RootProvider get disposed first, it will automatically dispose all attached ServiceProviderEngineScope objects.
RootProvider.Dispose();

@davidfowldavidfowlAug 20, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why cant this just noop internally?

@tarekghtarekghAug 20, 2022•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It is simpler. no?

More code, more bugs :-) imagine in the future we add anything to the ServicePrivder dispose method.

@eerhardteerhardtAug 22, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why cant this just noop internally?

By "just noop internally", you mean adding the if check in ServiceProvider Dispose?

@davidfowldavidfowlAug 22, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yep.

eerhardt
eerhardt approved these changes Aug 22, 2022
View reviewed changes

@eerhardteerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@tarekgh
tarekgh merged commit cb52e3b into dotnet:mainAug 22, 2022
@tarekgh

tarekgh commented Aug 22, 2022

Copy link
Copy Markdown
MemberAuthor

/backport to release/7.0-rc1

@github-actions

github-actionsBot commented Aug 22, 2022

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0-rc1: https://github.com/dotnet/runtime/actions/runs/2905770618

@github-actionsgithub-actionsBot mentioned this pull request Aug 22, 2022
Merged
@tarekgh

tarekgh commented Aug 22, 2022

Copy link
Copy Markdown
MemberAuthor

/backport to release/6.0

@github-actions

github-actionsBot commented Aug 22, 2022

Copy link
Copy Markdown
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/2905993688

@github-actionsgithub-actionsBot mentioned this pull request Aug 22, 2022
Merged
@ghostghost locked as resolved and limited conversation to collaborators Sep 21, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@davidfowldavidfowldavidfowl approved these changes
@eerhardteerhardteerhardt approved these changes

Assignees

@tarekghtarekgh

Labels

area-Extensions-DependencyInjection

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

DependencyInjectionEventSource Memory leak

3 participants

@tarekgh@davidfowl@eerhardt