Check if parent scopes are disposed when checking scope IsDisposed #1061
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thinking you didn't mean 1029 as the issue this fixes. (?) |
|
Ah, no I did not...I blame disconnect between brain and hands. Edited the comment (here's hoping Github relinks the PR). |
|
The things I'm afraid of with this (things to test around) are:
For example, the child scope checking to see if the parent scope is disposed... if someone does Thread 1: childScope.Resolve(); then without a lock, technically the child could see the parent as not disposed, then the disposal on the parent finishes, then the child still thinks the parent is alive. I'm not super familiar with how I mean, I'm sure you've already thought of all that, just figured for clarity that's the stuff I know I think about. |
|
With regards to the memory leak question, we already keep a reference to the parent lifetime scope, so there's no additional objects being tracked. I've reworked the changes a bit to re-use the existing ParentLifetimeScope reference for the disposal check. With regards to the memory barrier, the fact we're now repeatedly accessing I'll look at either updating or reusing the concurrency benchmark to get more data on this. |
alistairjevans
force-pushed
the
parent-dispose-check
branch
from
9ec6f89
to
afc2dae
Compare
|
I've run the new nested scope benchmark that simulates the potentially worrying request scopes + unit of work behaviour (after fixing the benchmarks in #1062). Results are as follows: BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18362
AMD Ryzen 5 2500U with Radeon Vega Mobile Gfx, 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.0.100
[Host] : .NET Core 3.0.0 (CoreCLR 4.700.19.46205, CoreFX 4.700.19.46214), X64 RyuJIT
DefaultJob : .NET Core 3.0.0 (CoreCLR 4.700.19.46205, CoreFX 4.700.19.46214), X64 RyuJIT
Develop
Parent Disposal Check
|
…e the scope is not disposed.
… reference and be closer to the lifetime scope.
alistairjevans
force-pushed
the
parent-dispose-check
branch
from
afc2dae
to
3a8ef4c
Compare
|
Rebased onto the latest develop (which includes the immutable changes). Updated benchmark reports are below. The branch changes come out slightly under, but within the margin of error for the benchmark. It appears as though the parent checking changes haven't had an obvious impact, particularly if you check the ConcurrencyNestedScopeBenchmark, which was the area of concern. ChildScopeResolveBenchmarkDevelop
Changes
ConcurrencyBenchmarkDevelop
Changes
ConcurrencyNestedScopeBenchmarkDevelop
Changes
DecoratorsDevelop
Changes
DecoratorsDevelop
Changes
DecoratorsDevelop
Changes
DecoratorsDevelop
Changes
DecoratorsDevelop
Changes
DecoratorsDevelop
Changes
DecoratorsDevelop
Changes
DecoratorsDevelop
Changes
DeepGraphResolveBenchmarkDevelop
Changes
EnumerableResolveBenchmarkDevelop
Changes
OpenGenericBenchmarkDevelop
Changes
PropertyInjectionBenchmarkDevelop
Changes
RootContainerResolveBenchmarkDevelop
Changes
|
tillig
approved these changes
Dec 22, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed change that fixes #1020 by making disposal exceptions consistently thrown.
This change uses a new
IsTreeDisposedmethod inDisposable, which checks up the tree of parent scopes to see if any are disposed.You then always get the correct disposal exception thrown if some part of the scope tree is no longer valid.
I've run the benchmarks on the change, and the effect appears to be minimal; I will supply the raw benchmark results on this over the next few days; thought I'd present the solution as an option.