Ambient scopes not supported by all DI's #334
Comments
|
Is there no way to just hack this into the abstraction instead of trying to change the current abstraction? We're in RC2 and tbh I don't see us changing something that fundamental, maybe we can just return the existing container in this implementation. It isn't accurate but it's better than nothing, works in the default case and can throw if you create more than one scope (in this impl). What about that? |
|
@davidfowl I haven't digged into every usage of DI in the ASP.NET stack, but is it only used for web requests? SimpleInjector wont publish a package that doesn't work 100%. With that said, I will probably make one. What static accesor is there for the current web request? Similar to I will store the scope in there and managed it within the I hope this solves the out-of-the-box experience for SimpleInjector working with ASP.NET 5. |
Nope.
Nope. We don't have any statics like that anymore. |
⛳ 👏 |
Unfortunately, the incompatibility in ambient scopes between DNX and Simple Injector is just one of the many incompatibilities, and I would say one of the least problematic one, because that one would be fixable by making some small non-breaking changes to Simple Injector. There are however, other -in my view unsolvable- incompatibilities. Besides my comment on the old discussion 41 I recently described the incompatibilities between the two APIs in this comment. |
|
Moving this to Backlog as we are not planning to make any changes for this at this time. |
|
I think Castle Windsor is more or less incompatible with this library for the same reason. I was trying to build an adapter for it and I didn't manage to get it fully compatible (I don't know much about the Windsor internals though). Since it is one of the main goals of this library to be replacable with our 'preferred' container, I really think you should put more time into this before RTM - because right now this argument is just not true IMO. You are locking out quite a few major DI frameworks. |
|
I don't see us changing this one. Ambient state is generally bad and we use it sparingly in asp.net core (logging scopes). We don't have any code that deepens on ambient state flowing anywhere (because there is a performance cost to it). Post v1 we can look at our usage and see if there's a same way to keep those containers working in asp.net scenarios. |
|
@davidfowl, please explain why having ambient state in the infrastructure is a bad thing? |
|
Are you asking me why ambient state is bad in general or why having an ambient request scoped container is bad? Ambient state is as bad as static state. .NET has ways to scope it to a thread or to an async call chain or via the logical call context. They are all just ways to avoid passing things around. ASP.NET in particular already has the http context and there's no good reason to introduce ambient state when we can just associate objects with a logical http request. You also pay the cost of ambient state flowing when in some cases you may not want it to flow (it's why ASP.NET 4.x uses the illogical call context). It's just a bad pattern in general, and while sometimes it can't be avoided, we do our best to avoid it where possible. |
|
If you want ambient state, you can always make an async local (or whatever level of ambientness you require) and use it. But we certainly won't be changing all of the code within our frameworks to assume there is an ambient scope available. |
|
Hi David, I'm just trying to learn a thing or two here. Can you elaborate on why ambient state is actually bad? Your previous statements didn't contain any explanation on why ambient context is actually bad. To sum up your current reasoning:
Can you explain what the problem is in "avoid passing things around", especially when ambient state is solely used in the application's Composition Root (the infrastructure)? |
|
Ambient state makes things harder to test and isolate. Lets say wanted to call 2 methods with a different container instance because you made 2 scopes (within the same async call chain). If that scope was ambient and baked into the infrastructure it would be impossible to do so. It also makes things harder to test as the caller needs to know that ambient scope is required for things to function, that dependency isn't explicitly stated.
I don't have to explain this to you, you wrote a dependency injection container and you always quote SOLID principles on everything you write. I'm not sure why you'd want to bake ambient state into anything (even the composition root) when it just isn't required. Like I said, what we have implemented is the most flexible pattern that makes no assumption on any ambient scoped container. If you want to expose a Just so we're clear on what we're both talking about the difference between: public void CompositionRoot()
{
var entryPoint = ServiceProvider.Current.GetService<EntryPoint>();
entryPoint.Invoke();
}
using(var scopedProvider = MakeScopeProvider())
{
ServiceProvider.Current = scopedProvider;
CompositionRoot();
ServiceProvider.Current = null;
}and: public void CompositionRoot(IServiceProvider provider)
{
var entryPoint = provider.GetService<EntryPoint>();
entryPoint.Invoke();
}
using (var scopedProvider = MakeScope())
{
CompositionRoot(scopedProvider);
}PS: I also think it's a bad pattern in general. |
You seem to imply that using Ambient State isn't SOLID which is IMO incorrect. As long as we encapsulate access to ambient state with adapters and we make this part of the application's Composition Root, the use of ambient state can very well be SOLID. You also claim that I want to “bake ambient state into anything” but I would never suggest or promote this. Keeping ambient state encapsulated and isolated in the Composition Root is something completely different to “baking ambient state into anything”. Furthermore, when we apply the SOLID principles, the solution can remain highly testable. The caller doesn't need to know about the existence of any ambient scope at all and we implement fake adapters that provide various ambient states during testing. You keep repeating your claim that ambient state is a "bad pattern in general" but have yet to provide any reasonable arguments to support this claim. To summarize:
From this we have to conclude that there’s nothing wrong with ambient state in general. Ambient state, as with any pattern, can be used incorrectly but I've yet to see any arguments that reliably demonstrate that ambient state, encapsulated in the Composition Root, is bad. This brings me to the point I’m trying to make here: It’s reasonable that you choose to not support ambient contexts in your own DI container, but please stop saying that ambient state is a "bad pattern in general" without providing the community with any reasonable arguments to validate your claim. You are a leading figure in our industry and many will take your word at face value. By making half-baked claims without supporting arguments, you are harming our industry. |
|
It's a leaky abstraction. You're right that you can reduce the impact by hiding it from some of the layers sure. But anything that needs to be aware of the ambient data is untestable. Just because you can hide it in some scenarios doesn't mean the pattern is SOLID. It forces all of the composite roots in the system to be aware of the ambient container. On top of that, depending on the level of ambience you end up with strange quirks like this: Warning: A lifetime scope is thread-specific. A single scope should not be used over multiple threads. Do not pass a scope between threads and do not wrap an ASP.NET HTTP request with a Lifetime Scope, since ASP.NET can finish a web request on different thread to the thread the request is started on. From the simple injector docs. My point is, even if you can hide it inside of the composition root and limit the exposure, quirky behavior usually follows because with ambient state, you lose the ability to express that state as part of the contract and sometimes that has unintended consequences. As a contrived example: using (container.BeginScope())
{
var scope1Foo = container.Resolve<IFoo>();
using (container.BeginScope())
{
var scope2Foo = container.Resolve<IFoo>();
// How do I resolve from the outer scope here?
}
}VS using (var scope1 = container.BeginScope())
{
var scope1Foo = scope1.Container.Resolve<IFoo>();
using (var scope2 = container.BeginScope())
{
var scope2Foo = scope2.Container.Resolve<IFoo>();
// How do I resolve from the outer scope here?
var scope1Foo2 = scope1.Container.Resolve<IFoo>();
}
}It's much clearer what is happening here as the object reference to the scoped container is directly exposed and not hidden from the code. |
|
Hi David,
Of course we can just “disagree”, but I’d much rather evolve a discussion leading to meaningful understanding based on ‘solid’ arguments, instead of you –again– making rash statements.
|
Thanks for the tip. |
|
I need some clarification on the statements made in this thread. In the applications I write, I try to follow the SOLID principles. Your statements in this post seem to point out that I’m doing something wrong. I’m hoping you can educate me in improving my application design. I have several examples of ambient state in the applications I maintain. I always thought that I was following SOLID, because I hide that state behind abstractions. I’m hoping you can reflect on my design and show me where my abstractions are leaking and where this baked state causes problems with for instance unit testing. One of the most basic examples I could find in my code base which is also widely used throughout several parts of my applications is a service, adapter or whatever you like to call it for providing information about the request’s current user. I use an abstraction that is similar to the following: public interface IUserContext
{
string CurrentUser { get; }
}
Depending on the environment, I have these implementations amongst others: private sealed class WindowsUserContext : IUserContext
{
public string CurrentUser => WindowsIdentity.GetCurrent().Name;
}
private sealed class HttpUserContext : IUserContext
{
public string CurrentUser => HttpContext.Current.User.Identity.Name;
}Deep down below in the object graph, I often need access to the current user, for instance when performing an (database) operation. These are things like permission checking, audit trail, etc. What you’re implying, at least that is my interpretation, is that runtime state should flow through the system, which I think means that all public methods need a But isn’t passing of the I’m also worried about the sweeping changes that this would cause me to make throughout my system, every time some low level component suddenly requires the use of the current user. How would I prevent these sweeping changes? I also thoroughly test my application by using both unit and integration tests and never had any problems before in getting full coverage both in the consumers of this I’m very interested in your thoughts and comments. |
This is super common and the pattern you have above does work and is testable. But let me play devils advocate, why is the current user special? Why don't you make a bunch of other things in your system ambient so you don't have to worry about passing them around? You can always hide each of those things behind another interface so consumption is ignorant to the fact that you are passing things via this ambient state right?
Passing arguments to things that require those arguments is the opposite of leaky. Its like saying cancellation tokens are leaky when you need to pass them to something that needs to be cancelled (they're annoying but not leaky, they are honest 😄 ). But! As @dotnetjunkie says, your components are fine because you are properly abstracting them away from the ambient state. Those components themselves are very testable because the ambient state is an implementation detail. The argument I was making was that there are specific components that need to deal with the fact that ambient state exists (the entire discussion stemmed from the fact that ambient scopes are the default in simple injector). The "composition root", or rather anything that itself needs to create, pass or maintain a scope would need to directly access the ambient scope. Even worse, there are cases where you might need to access a different scope (request scope vs transaction scope vs thread scope) and without access to the object, you've lost the ability to pick which state you care about. Now we might be in the weeds talking about "infrastructure" but any code that needs to be explicit about calling PS: I would take advance from @dotnetjunkie over me WRT this area. I just write frameworks 😉 , not real code. |
|
Thank you for taking the time to answer my questions so swiftly.
I’m having a hard time imagining concrete cases where access to a different scope is needed. Can you give some concrete example where access to a different scope is a necessity? |
Anywhere that needs to call https://github.com/aspnet/SignalR/blob/dev/src/Microsoft.AspNetCore.SignalR/HubEndPoint.cs#L91 |
|
I’ve been thoroughly browsing the code bases your links referred to, but I think I’m missing something here, because none of your linked examples seem to show how a different (outer or parent) scope is accessed while running inside a (inner) scope. The recurring pattern seems to be that a This makes me really confused. What am I missing here? So concerning your earlier statement:
Can you elaborate on how the presented examples show the problem you described in that statement? |
I didn't go through the examples to show how "outer" scopes are accessed from inner scopes. I'm not sure if they do that in those above examples. My point was merely that ambient state makes that much harder as you've completely hidden the state from the consumer. When the scope is explicit, you can just pass it along explicitly without worrying about where it may or may not leak to. |
|
After trying to digest your latest reaction I came to the conclusion that the difference in our field of work (framework developer vs application developer) is probably too big to clearly understand each other’s problems, questions and design challenges.
As an application developer I would reflect on this with the following: A design where we pass on some framework defined scope object explicitly through application code, would violate the SOLID principles and is just a case of a missing abstraction. But since I have no experience with building frameworks myself, I’m not sure whether such statement holds when building frameworks or not. Up until now I didn’t read any convincing arguments that lead me to believe that ambient state actually is “bad in general” and shouldn’t be used, especially when kept inside the Composition Root. There might be special situations, maybe especially in the context of writing frameworks where there are better solutions opposed to using ambient state but at this point I come to the conclusion that there is nothing wrong with my current application design after all. I’m also led to believe there is nothing wrong with DI containers that use and provide ambient state like Castle Windsor and Simple Injector do. Both libraries are designed and supported by two highly skilled professionals which both actively promote SOLID and well-designed code and it would be a big disqualification to say that their libraries forces application developers into violating good design principles. I see no problem at all in the usage of a container in the composition root, which has always been the guidance. One should always choose the best tool for the job at hand and in my current design the use of ambient state, encapsulated in the Composition Root, does a really good job. Thank you for your time and patience. |
|
No problem. |
|
From the previous discussion, even though David Fowler may not 'like' the use of ambient state, it is important to stress that there is nothing inherently wrong with the use of ambient state encapsulated within the Composition Root. Ambient state, when encapsulated within the Composition Root:
I will discuss each of the above in more detail below. Ambient state, when encapsulated within the Composition Root: Is wholly testable.As stated before, and even confirmed by David in response to @TheBigRic, the use of ambient state can be tested and when ambient state is hidden behind SOLID abstractions, the consumers of these abstractions can be tested without requiring instances of ambient state. Ambient state, when encapsulated within the Composition Root: Does not violate SOLIDAmbient state is state. State can (and should) be encapsulated behind adapters that follow the SOLID principles. Ambient state, when encapsulated within the Composition Root: Does not leak.Passing runtime state through various method calls of the object graph -as David suggested earlier- will cause abstractions to leak implementation details. For instance, take a look at the following abstraction with a simple implementation: public interface ICustomerRepository
{
void Save(Customer entity);
}
public class SqlCustomerRepository : ICustomerRepository
{
private readonly IUnitOfWork uow;
public CustomerRepository(IUnitOfWork uow) {
this.uow = uow;
}
public void Save(Customer entity) {
this.uow.Save(entity);
}
}Imagine that we need to alter this public class SqlCustomerRepository : ICustomerRepository
{
public void Save(Customer entity) {
this.uow.Save(entity);
this.uow.Save(new AuditTrail {
EntityId = entity.Id,
Type = "Save",
CreatedOn = DateTime.Now,
CreatedBy = ???
});
}
}The problem here is that the public interface ICustomerRepository
{
void Save(Customer entity, Guid currentUserId);
}This minor alteration has caused sweeping changes to ripple through the call stack, since all direct and indirect consumers will need to pass this value on. This causes many other application abstractions to be altered in favor of this 'minor change'. In SOLID terminology: this is both an Open/Closed Principle violation and a Dependency Inversion Principle violation. Now, imagine that we did go through the painful process of making these sweeping changes through the application and then another request comes in: a change that asks us to store the If, however, we were to inject an class CustomerRepository : ICustomerRepository
{
private readonly IUnitOfWork uow;
private readonly IUserContext userContext;
public CustomerRepository(IUnitOfWork uow, IUserContext userContext) {
this.uow = uow;
this.userContext = userContext;
}
public void Save(Customer entity) {
this.uow.Save(entity);
this.uow.Save(new AuditTrail {
EntityId = entity.Id,
Type = "Save",
CreatedOn = DateTime.Now,
CreatedBy = this.userContext.UserId
});
}
} The use of the Ambient state, when encapsulated within the Composition Root: Will not cause us to "lose the ability to express that state as part of the contract".The statement that we can't express state as part of the contract is unfounded. On the contrary; we get more options. In case we find that contextual information is an implementation detail, we can encapsulate it behind abstractions to prevent changes from rippling through the system. If, however, we find that this runtime data should be part of the public contract of the application, we can still design it as such. Nobody said that all information should be stored in ambient state and nothing should be passed through method calls. One of the arguments David used was that "It forces all of the composite roots in the system to be aware of the ambient container". A Composition Root is the entry-point of the application and is, therefore -by definition-, aware of every aspect of the application. It has intrinsic knowledge about the selected DI Container, application framework (e.g. ASP.NET Core MVC) and the platform the application is running on. It is a non-issue for the Composition Root to be aware of ambient state. It might be that while using ambient scoping, it becomes harder to access the parent scope, while you're inside the context of a nested scope, such as David's contrived example shows. It is however, a very uncommon use case and it is actually pretty hard to come up with a valid scenario for such a thing. Although the .NET Core DI container does allow this scenario, it's very unlikely that that anyone is currently using this 'feature'. David showed 4 examples that all follow the 'normal' way of working; a way that Simple Injector and Castle Windsor support as well. I would say that this 'limitation' is non-existent. Accidentally resolving services from a parent scope is even a very common source of bugs in applications that use DI Containers. ConclusionYou should definitely not let application code depend directly on ambient state, just as you shouldn't let application code depend directly on the DI Container. However, ambient state, when encapsulated within the Composition Root, is testable, maintainable and does not violate any of the SOLID principles and, therefore, is not "bad in general". |
|
A layer of indirection solves everything in computer science 🤣. You're right, we can hide the ambient state behind an abstraction except for the things that need to care about it but TBH that not a realization. Thanks for the discussion gentlemen. Edit: IMO, the ambient state should be decoupled from the container itself and the framework infrastructure should decide how objects are affinitized. Most containers either let you change how the internal scoping works via some scope provider interface, or even more simply by avoiding any notion of ambience and leaving that up to the caller, which IMO is the simpler thing to do. |
So, I'm having a discussion with @dotnetjunkie about implementing a dnx adapter for SimpleInjector (simpleinjector/SimpleInjector#156).
He is not a fan of implementing an abstraction, it is a conforming container anti-pattern. I've argued that if he doesn't create a dnx adapater, SimpleInjector will simply not be a choice with users when doing ASP.NET 5 projects. I believe (and he can correct me if I am wrong) that the ultimate issue comes down to Microsoft's use of ambient scopes.
So, my question is this. When thinking of the common denominator of features between containers, is this actually needed?
Within the ASP.NET stack, are scopes only used within the context of web requests?
What about adding two kinds of scopes:
IServiceScopeto resolve a service. This is how the ASP.NET DI currently does it.If the ASP.NET request pipeline is currently the only thing using scopes, maybe it can be changed to use global scopes, leaving the local approach in-tact, but just not used. If we do this, SimpleInjector can support web requests, while explicitly declaring that they do not support local scopes. The maintainer of SimpleInjector doesn't have any intention of ever supporting local scopes, and it could simply be a known limitation of using a SimpleInjector dnx adapter. Of course, provided that the core libraries for ASP.NET don't depend on any local scopes. We don't want the SimpleInjector dnx adapter to be broke out-of-the-box.
Any thoughts? I don't think @dotnetjunkie is budging ;) SimpleInjector is one of the fastest DI's, and it will break my heart to see it not supported.
The text was updated successfully, but these errors were encountered: