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

CopyOnWriteRegistry possibly not correctly disposed #746

Closed
dozm opened this issue May 5, 2016 · 2 comments
Closed

CopyOnWriteRegistry possibly not correctly disposed #746

dozm opened this issue May 5, 2016 · 2 comments
Labels

Comments

@dozm
Copy link

dozm commented May 5, 2016

at: Autofac/src/Autofac/Core/Registration/CopyOnWriteRegistry.cs line:59
_readRegistry?.Dispose();
Whether it should be:
_writeRegistry?.Dispose();

this code will not throw an exception,Since you did not call _componentRegistry.Dispose() in LifetimeScope.Dispose()

@tillig
Copy link
Member

tillig commented May 23, 2016

This gets a little complicated, but I think you might be right.

Some research/notes:

The only place we create CopyOnWriteRegistry is during LifetimeScope.BeginLifetimeScope like here. The point of the CopyOnWriteRegistry is to isolate new registrations that get made in lifetime scope configuration actions from a parent component registry.

The CopyOnWriteRegistry gets the parent component registry as a constructor parameter (the "read registry") and then creates a "scope restricted registry" when writes (new registrations) are made.

While the parent lifetime scope (or container) is responsible for disposing of the root component registry, the creation of these disposable scope restricted registries doesn't actually appear to get handled - the _writeRegistry?.Dispose() as noted in the original issue isn't called.

That said, it doesn't look like anything actually ever does dispose of these CopyOnWriteRegistry instances. The registry isn't added to the new lifetime scope's Disposer so I'm not sure where that happens, if at all. I haven't hooked a debugger up to it to verify.

What it looks to me needs to happen:

  • The CopyOnWriteRegistry needs to be added to the Disposer of the child lifetime scope so new registrations will be actively disposed.
  • The _writeRegistry should be disposed, not the parent _readRegistry.
  • A little doc should be added to LifetimeScope.CreateScopeRestrictedRegistry to indicate how this all works.

@alexmg Does this sound right to you?

@tillig tillig changed the title CopyOnWriteRegistry.Dispose() error CopyOnWriteRegistry possibly not correctly disposed May 23, 2016
@tillig tillig added the bug label May 23, 2016
afabre added a commit to afabre/Autofac that referenced this issue Jul 23, 2017
When a child scope is created, along with its registry, we need to keep track
of what needs to be disposed of later. The registry is part of those things.

The registry passed in the constructor doesn't always fall in that category
(think root scope), and hence the registry is added in the BeginLifetimeScope
methods.

Issue autofac#746
afabre added a commit to afabre/Autofac that referenced this issue Jul 23, 2017
When the registry hasn't been touched, nothing needs to be disposed. Only when
the registry has been modified, and hence a new (write) registry allocated,
should the registry be disposed. This is determined by the _writeRegistry
field being non-null.

Issue autofac#746
afabre added a commit to afabre/Autofac that referenced this issue Jul 23, 2017
When the registry hasn't been touched, nothing needs to be disposed. Only when
the registry has been modified, and hence a new (write) registry allocated,
should the registry be disposed. This is determined by the _writeRegistry
field being non-null.

Fixes Resharper warnings in that class.

Issue autofac#746
afabre added a commit to afabre/Autofac that referenced this issue Jul 23, 2017
When a child scope is created, along with its registry, we need to keep track
of what needs to be disposed of later. The registry is part of those things.

The registry passed in the constructor doesn't always fall in that category
(think root scope), and hence the registry is added in the BeginLifetimeScope
methods.

Issue autofac#746
alexmg added a commit that referenced this issue Jul 24, 2017
Fixed #746: Registrations added to a nested lifetime scope were not being disposed.
@alexmg
Copy link
Member

alexmg commented Jul 25, 2017

This fix is included in 4.6.1 which has been pushed to NuGet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants