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

Disposing scopes more than once should work #50852

Merged
merged 1 commit into from
Apr 8, 2021
Merged

Conversation

davidfowl
Copy link
Member

  • Recently introduced a regression where disposing DI scopes multiple times throws.

See dotnet/aspnetcore#31576 where this was found.

- Recently introduced a regression where disposing DI scopes multiple times throws.
@ghost
Copy link

ghost commented Apr 7, 2021

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

Issue Details
  • Recently introduced a regression where disposing DI scopes multiple times throws.

See dotnet/aspnetcore#31576 where this was found.

Author: davidfowl
Assignees: -
Labels:

area-Extensions-DependencyInjection

Milestone: -

@davidfowl davidfowl requested a review from eerhardt April 7, 2021 16:21
@@ -176,6 +176,12 @@ private void ClearState()
// try to return to the pool while somebody is trying to access ResolvedServices.
lock (_scopeLock)
{
// Don't attempt to dispose if we're already disposed
if (_state == null)
Copy link
Member Author

Choose a reason for hiding this comment

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

I could also check _disposed.

Copy link
Member

Choose a reason for hiding this comment

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

Why do we only set _state to null below if Return returns true? We don't want to always null it out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Super weird edge case around root scopes vs child scopes #50463 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add the comment to the code, so we remember next time?

Copy link
Member

Choose a reason for hiding this comment

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

Note, it also doesn't get nulled out when the pool is full.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's in the comment below this statement.

@davidfowl
Copy link
Member Author

This is why we need nullability checks on 😄

@davidfowl
Copy link
Member Author

What's the deal with the CI, is it on fire?

@davidfowl
Copy link
Member Author

/azp run runtime

@davidfowl
Copy link
Member Author

/azp run runtime-staging

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidfowl
Copy link
Member Author

/azp run dotnet-linker-tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidfowl
Copy link
Member Author

/azp run runtime-dev-innerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidfowl
Copy link
Member Author

Merging this one as I re-ran 5 times and the legs seem to be on 🔥 . This is blocking asp.net core grabbing new deps.

@davidfowl davidfowl merged commit 597a5bc into main Apr 8, 2021
@stephentoub stephentoub deleted the davidfowl/double-dispose branch April 8, 2021 15:16
@ghost ghost locked as resolved and limited conversation to collaborators May 8, 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.

3 participants