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
Update log4net module to handle registrations in nested lifetime scopes #59
Conversation
protected override void Load(ContainerBuilder builder) | ||
{ | ||
// Handle registrations in nested lifetime scopes. | ||
builder.RegisterBuildCallback(c => c.ChildLifetimeScopeBeginning += OnChildLifetimeScopeBeginning); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should register for the container creating child scopes but I don't think it handles those children creating scopes, like...
using(var scope1 = container.BeginLifetimeScope())
{
// scope1 should have things attached
using(var scope2 = scope1.BeginLifetimeScope())
{
// I don't think scope2 will be registered
}
}
One of the reasons the example isn't super robust is... it's an example, and we don't have unit tests or anything to verify that a super rich example like this actually handles everything.
Additional related issues:
- Module.AttachToComponentRegistration() does not detect new registrations in nested scopes Autofac#218 - It might be better to figure out how to properly handle nested lifetime scopes than enhance this example.
- Nested ContainerBuilder created with BeginLifetimeScope shares BuildCallback list with its parent ContainerBuilder Autofac#912 - If this updated example does work, it may be because of this bug where nested container builders incorrectly share the build callback list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right - it wasn't handling children scopes. I've updated PR to handle that.
So if I understand correctly we could tackle this in two different ways:
|
We have, thus far, not really been interested in creating a specific module like this and having to support it. That's why it's shown as an example rather than Yet Another Integration Package. It's not "core functionality" so wouldn't really go in Autofac core, but we also put out a call for additional owners to help out and we haven't had any takers. If this is an interesting area to you, I'd recommend creating a package with a generic, pluggable, supported module like this and releasing it. We'd be happy to link this doc to your package. Barring that, I'm reluctant to really take this PR for the docs because it's an example, not a "supported solution to handle every need." This shows you how you can do things, but since there's no testing and there's no actual package, it's hard to say "this works for everything." I'd be much more inclined to take a PR for a note that says something to the effect of, "Note: This is an only an example and may not cover every possible use case. For example, nested scopes are not necessarily properly handled." Further, if we do fix those defects, that changes how this module needs to be implemented. For example, if the root issue here (autofac/Autofac#218) is fixed, then the handling of child scopes in this PR would then have to be removed because it would instead double-handle the issue. Or if the other issue I linked (autofac/Autofac#912) gets fixed, I don't know how it affects this build callback registration, where the child already [incorrectly] shares the build callback list with the nested scope. And it's hard to say if the example will keep working when it gets too detailed because there aren't tests and it's just an example. Which kinda takes us back to the beginning. I know we've had a few people asking for a package with a generic module like this in it, something like an abstract class where you could have a public abstract class FactoryModule<T>
{
/* all the other stuff, except abstract methods to indicate if you can handle the parameter... */
public abstract bool CanHandleParameter(ParameterInfo parameter, IComponentContext context);
public abstract T CreateInstance(ParameterInfo parameter, IComponentContext context);
} That may not be the right breakdown, or naming, or whatever, but you get the idea. |
Original example fails when a new dependent service introduced in nested scopes.