Closed SoaresMG closed 6 years ago
Actually I could create a new context on those situations using the ContextFactory but if feels bad. This way it would approach the service locator pattern when it wouldn't be needed.
You are right, there is nothing built-in to do this. A scope is a method of isolation, and sharing instances between isolated bubbles is not very common.
There are two ways around this: either you change your code, or you create a custom lifestyle.
Let's start with the former. Instead of resolving an IOperationScope
from the container, you can instead pass it along anytime you create a new nested scope. You could use the Scope
class itself as dictionary for such data container, for instance by calling scope.SetItem(key, value)
.
Your second option is to create a custom ScopedLifestyle
implementation that only caches instances in the outer scope. Here is a working example:
public class OuterScopedLifestyle : ScopedLifestyle
{
public OuterScopedLifestyle() : base("Outer Scoped") { }
// By making the length of this lifestyle one bigger than Lifestyle.Scoped, prevent
// OuterScopedLifestyle instances from depending on Lifestyle.Scope instances,
// which could obviously lead to problems.
public override int Length => Lifestyle.Scoped.Length + 1;
protected override Func<Scope> CreateCurrentScopeProvider(Container container) =>
() => this.GetOuterScope(Lifestyle.Scoped.GetCurrentScope(container));
protected override Scope GetCurrentScopeCore(Container container) =>
this.GetOuterScope(Lifestyle.Scoped.GetCurrentScope(container));
// Walk the scope-stack to find the outer-most scope.
private Scope GetOuterScope(Scope scope)
{
if (scope == null) return null;
var parentScope = this.GetParentScope(scope);
while (parentScope != null)
{
scope = parentScope;
parentScope = this.GetParentScope(scope);
}
return scope;
}
// Unfortunately, the Scope.ParentScope property is internal in Simple Injector v4.0.
// We need to use reflection to get to it.
private readonly PropertyInfo parentScopeProperty =
typeof(Scope).GetProperties(BindingFlags.Instance | BindingFlags.NonPublic)
.Single(p => p.Name == "ParentScope");
private Scope GetParentScope(Scope scope) => (Scope)this.parentScopeProperty.GetValue(scope);
}
I hope this helps.
Thank you for your answer, I thought on something like this but you made it in 5 minutes 😄
Just one more question, would I need to specify this Custom scope while registering the service?
container.Register<IOperationScope, OperationScope>(Lifestyle.Scoped);
to
container.Register<IOperationScope, OperationScope>(new OuterScopedLifestyle());
and when I'm launching a new thread, do I need to modify it?
using (AsyncScopedLifestyle.BeginScope(container))
By the way, can you explain the Length
property? In which way is it used?
Quick update:
It throws an errors when trying to initialize the IOperationScope
SimpleInjector.ActivationException Cannot access a disposed object. Object name: 'SimpleInjector.Scope'.
The scope
seen in the screenshot above seems to be disposed when it tries to get the instance.
would I need to specify this Custom scope while registering the service?
Since OuterScopedLifestyle
uses Lifestyle.Scoped
, you need to set the Container.Options.DefaultScopedLifestyle
to the application's default lifestyle. After that, you can use Lifestyle.Scoped
as lifestyle for 'normal' scoped registrations, and use OuterScopedLifestyle
explicitly, for that one registration that requires a unique instance per set of nested scopes. Since OuterScopedLifestyle
is built on top of Lifestyle.Scoped
(compared to building a completely separate cache for scopes), you can simply begin a scope for the default scoped lifestyle, and OuterScopedLifestyle
will function within that scope.
This all means you can use OuterScopedLifestyle
as follows:
// Registrations:
var container = new Container();
// Set your default scoped lifestyle for scoped registrations
container.Options.DefaultScopedLifestyle = new AsyncScopedLifestyle();
// Register 'normal' scoped registrations
container.Register<ITaskFinder, MyTaskFinder>(Lifestyle.Scoped);
// Register 'special' scoped registration
container.Register<IOperationScope, MyOperationScope>(new OuterScopedLifestyle());
// Usage: (exactly as you're doing already)
IOperationScope s1;
IOperationScope s2;
using (AsyncScopedLifestyle.BeginScope(container))
{
s1 = container.GetInstance<IOperationScope>();
using (AsyncScopedLifestyle.BeginScope(container))
{
s2 = container.GetInstance<IOperationScope>();
}
}
Assert.AreSame(s1, s2);
By the way, can you explain the Length property? In which way is it used?
The only consumer of the Length
property is Simple Injector's Diagnostic sub system. By letting the Length
property return a value that is higher than that of the normal scoped lifestyles (such as AsyncScopedLifestyle
and ThreadScopedLifestyle
) it allows Simple Injector Diagnostics to detect lifestyle mismatches. This ensures that a OuterScopedLifestyle
component can only depend on components that are either Singleton
or OuterScoped
, but not Async Scoped
or Transient
.
The scope seen in the screenshot above seems to be disposed when it tries to get the instance.
Can you provide me with a full strack trace and a minimal example that reproduces this issue?
Do you have any jumpstart project so I could use to reproduce this myself and then show you?
Let's start with the stack trace. Perhaps that reveals enough info. Can you post the full stack trace of the exception and its inner exceptions?
Ok Stacktrace
SimpleInjector.ActivationException: Cannot access a disposed object.
Object name: 'SimpleInjector.Scope'. ---> System.ObjectDisposedException: Cannot access a disposed object.
Object name: 'SimpleInjector.Scope'.
at SimpleInjector.Scope.ThrowObjectDisposedException()
at SimpleInjector.Scope.GetInstanceInternal[TImplementation](ScopedRegistration`1 registration)
at SimpleInjector.Scope.GetInstance[TImplementation](ScopedRegistration`1 registration, Scope scope)
at SimpleInjector.Advanced.Internal.LazyScopedRegistration`1.GetInstance(Scope scope)
at lambda_method(Closure )
at SimpleInjector.InstanceProducer.GetInstance()
--- End of inner exception stack trace ---
at SimpleInjector.InstanceProducer.GetInstance()
at SimpleInjector.Internals.ContainerControlledCollection`1.<GetEnumerator>d__24.MoveNext()
at System.Linq.Enumerable.WhereSelectEnumerableIterator`2.MoveNext()
at System.Linq.Enumerable.<UnionIterator>d__67`1.MoveNext()
at System.Linq.Enumerable.WhereEnumerableIterator`1.MoveNext()
at System.Linq.Enumerable.Any[TSource](IEnumerable`1 source)
at Connector.SDK.Services.Jobs.TaskFinder.TaskFinder.Get(String Code, ErpType Erp) in D:\VS\Projects\EP\DEV\ConnectorV2\Connector.SDK\Services\Jobs\TaskFinder\TaskFinder.cs:line 22
at Connector.SDK.Services.Jobs.Management.TaskManager.<>c__DisplayClass9_0.<<RunAsync>b__0>d.MoveNext() in D:\VS\Projects\EP\DEV\ConnectorV2\Connector.SDK\Services\Jobs\Management\TaskManager.cs:line 85
What does the TaskFinder
look like? It seems to iterate over an injected collection of dependencies.
It iterates through the task collection to find the one with the Job Code that the user wants to run.
This collection is registered as RegisterCollection
.
and registered as
Do you want me to send the code instead of images?
No, images are just fine.
Not related to this problem, but this TaskFinder
implementation might become a performance problem. How many ITask
and ITaskAsync
implementations do you have at the moment?
Right now 20 but it will be perhaps 100-200... It wastes like ~500ms and since the user doesn't have to wait for it, I didn't really care.
If you have 200 tasks, you'll potentially end up with the creation of 600 tasks with each call to TaskFinder.Get
and each task might potentially create many objects of itself. Your TaskFinder
has an O(n) chararacteristic. It's good to be aware of this.
If this class becomes a performance bottleneck, please come back here so we can discuss an optimized solution.
The TaskFinder
by itself is fine. At the moment you call foundTasks.Any()
it starts to iterate the collection, which means Simple Injector starts resolving components for you. Since somewhere in the chain there is a scoped dependency, Scope.GetInstance
is called.
For some unknown reason that Scope
has been disposed. I can't see why this would be.
You'll have to find out where the Scope is disposed. The simplest way to do this is to register a disposable dummy object as Lifestyle.Scoped
, resolve it directly after creating a new scope and putting a break point inside its Dispose
method:
// Dummy class
public class Dummy : IDisposable
{
public void Dispose() { Debugger.Break(); }
}
// Registration
container.Register<Dummy>(Lifestyle.Scoped);
// Use
using (AsyncScopedLifestyle.BeginScope(container))
{
container.GetInstance<Dummy>();
// other code
}
Now, when you run the application, the break point should be hit before or during the call to TaskFinder.Get
. When you inspect the strack trace, you should be able to find out at what point the scope is disposed.
If you have 200 tasks, you'll potentially end up with the creation of 600 tasks with each call to TaskFinder.Get and each task might potentially create many objects of itself
I was aware of this when I did, however I didn't have the time to make this adjustment.
If this class becomes a performance bottleneck, please come back here so we can discuss an optimized solution.
Seems great! Where do you want to open this discussion?
You'll have to find out where the Scope is disposed
I'll implement IDisposable
on my IOperationScope
and try to find where it's being disposed.
I assumed the message was saying that the first scope Container
was being disposed, but it doesn't really make sense.
I'll give feedback asap.
You might get different results for your dummy, whem its registered using outerscoped vs Lifestyle.Scoped. might want to try both.
Using OuterScoped: It seems that it's being disposed while ending the API request.
With AsyncScoped: It doesn't dispose, but it gives me an error further in the code saying that IOperationScope.OperationId is null
which is the expected, since I'm not passing it down.
Oh I see... This OuterScope is going to the AsyncScope on the Request level. The request is already disposed.
AsyncScope
(Middleware in my last comment)ITask
Thread 2
with ITask
executionAsyncScope
inside ThreadThread 1 IOperationScope
Does it make sense?
How can I check if Scope
is disposed?
Changed the method to get the last Non-disposed Scope
But now it just gives me an IOperationScope
with the OperationId
null
I may have max 3 nested AsyncScopes
I do this since I do SaveChanges
inside HttpClientLogger
(T
) and I don't want it to save things that it doesn't belong to.
Inside this HttpClientLogger
I call IOperationScope
which I was expecting to inherit from:
- Thread Specific Scope
But it seems that it's getting from:
- Humble class
It would be easier If I could name scopes. Then I could see which scope it's being retrieved.
Sorry for the delay, but a man has to sleep from time to time :)
It seems that it's being disposed while ending the API request.
Nested scopes are related to each other when they are in the same logical context (which is the same thread in case of the ThreadScopedLifestyle
and the same asynchronous context in case of the AssyncScopedLifestyle
). This means that whenever you dispose a scope, all its inner scopes will go out of scope as well.
So you can't continue the execution of an inner scope, when you destroyed the outer scope. Instead, if it is your intention to spin of an isolated operation that continues even if the parent operation, thread or request ends, you will have to start a new thread. This new thread will get its own scope, which is the outer-most scope for that thread. The relationship with the original operation is gone.
This implies however that the OuterScopedLifestyle
is useless in your case, and you will have to manually move the correlation information (such as your OperationId
) manually to the new thread, and either attach it to the IOperationScope
, or you could use the Scope
itself as dictionary by using its GetItem
and SetItem
methods.
But I start a new thread each time I run a new Job
Isn't it enough to decouple from the main thread's scope?
That should indeed be enough. There should be no relationship (and interference) with the main thread.
But you are experiencing different behavior?
Strange thing... It all works if I place breakpoints on specific places. It seems to be concurrency then, it's the typical symptom...
I'll try to give you a running example
@dotnetjunkie Hey, just created a simple repo with the error. SimpleInjectorOuterScopeTest
Connector.Server
> Controllers
> TestController.cs
> Run
I suggest a BP on Line 76 of TaskManager.cs
, it's when the new thread inits.
Thank you for your time. I will keep trying to find the problem.
PS If you want the registrations just go to SDK
> Injector
> Mapping.cs
I've been pulling my hairs out for the last 3 hours, trying to understand why things are working so differently than I expected this to work. It's as if things have changed dramatically in a newer installment of .NET, but it just seems my mind is playing tricks on me.
It seems that a task created using Task.Run
will always get a copy the call context (which is what Simple Injector uses under the covers to transport scopes), which means it will get the original active Scope
with it. This means that within the task, calling Lifestyle.Scoped.GetCurrentScope
will result in the original scope.
When using ThreadPool.QueueUserWorkItem(_ => {...})
or new Thread(() => {...}).Start()
on the other hand, might give you a clean sheet, but it depends on the environment you're running in. Within the ASP.NET application I created locally, I got a clean sheet, which means that Lifestyle.Scoped.GetCurrentScope
will return null, but if you run that same code from within a Console application, you'll notice that you actually do have access to the original call context.
Within your ASP.NET application however, I see the same behavior as with the Console application, which means that even within a new Thread
, it is able to access the call context, from the code that started it.
Another problem with both ThreadPool.QueueUserWorkItem
and new Thread
is that, AFAIK, they don't accept any async
operations, which will possibly cause them to block more threads, and lower performance (which might not be a problem for your application).
I'm a bit dazzled by this different behavior between applications and frameworks, which is something I don't fully understand.
The reason you got the ObjectDisposedException
when iterating items from the TaskFinder
is because:
TaskFinder
depends on collection of ITask
and IAsyncTask
and (some of) their implementations depend on IOperationScope
, which is registered with OuterScopeLifestyle
.Task
runs in parallel with the original request, which allows the original request to finish before the Task
is finished.OuterScopeLifestyle
gets the scope that wraps the request, it can fetch a scope that is possibly already disposed, which is exactly what happens in your case.Together this results in an ObjectDisposedException
.
My advice would therefore be to:
Task.Run
, since it allows async
operations.Task
in a new AsyncScopedLifestyle
to ensure isolation.OuterScopedLifestyle
, since it is unsuited for situations where the new operation runs in parallel, instead of simply asynchronously.Task.Run
, either using the Scope
(using SetItem
/GetItem
) or with the IOperationScope
construct you are already using, where IOperationScope
is registered as normal AsyncScopedLifestyle
.My workaround was to just grab the older IOperationScope
before creating a new scope and then replace it in the freshly created scope.
I do this in 3 class's it's not a major problem for now. Just wanted to understand if this was possible.
But it's strange that when running it, if you try multiple consecutive times, it will eventually work. That's why I thought it was concurrency problems.
Oh well, Software Vs Developers.
What about the shitty IEnumerable<ITask>
collection implementation?
How can I do this without major performance concerns?
And... thank you your time! It's a pleasure to learn with someone who has more experience and knowledge.
What about the shitty
IEnumerable<ITask>
collection implementation?
Here's that code again (makes it easier to talk about):
public (bool IsAsync, object Task) Get(string Code, ErpType Erp)
{
var foundTasks = tasks.Where(x => x.Code == Code && x.Erp == Erp);
if (!foundTasks.Any())
throw new Exception($"Task {Code} not found.");
if (foundTasks.Count() > 1)
throw new Exception($"Task {Code} has {tasks.Count()} entries.");
return foundTasks.Select(x => (x.IsAsync, x.Task)).Single();
}
Well, first of all, you iterate the IEnumerable
three times (first time with Any()
, second time with Count()
, and third time with Single()
). Since enumerables in Simple Injector are streams, it means that every time you iterate the collection, its instances are resolved. Since these instances are registered as Transient
, it means each instance is potentially created 3 times.
You can already reduce the cost by a factor of 3 by materializing the filter to an array:
var foundTasks = tasks.Where(x => x.Code == Code && x.Erp == Erp).ToArray();
This still keeps the performance O(n), which means it gets slower with every new task added to the system, but this might be enough.
Ideally however, you would like to get to a characteristic of O(1), which means that independently of the number of tasks in the system, the performance is constant.
So let me start by suggesting a small change in your design, that simplifies your tasks and as extra benefit makes it easier to get this O(1) characteristic.
I would suggest changing your ITask
and ITaskAsync
abstractions to the following:
public interface ITask
{
TaskStatus Execute();
}
public interface ITaskAsync
{
Task<TaskStatus> Execute();
}
In other words, I suggest removing the Code
and Erp
properties from the abstractions. Reason for this is that their values don't seem to be related to a task instance, but rather to the type itself, as the CompositeTypeTask
seems to suggest:
public class CompositeTypeTask : ITaskAsync
{
...
public string Code => Enums.Jobs.CompositeTypes;
public ErpType Erp => ErpType.AX;
public async Task<TaskStatus> Execute() => ...
}
Instead of putting the properties on the interface, you can put them on the type using an attribute:
[Task(Enums.Jobs.CompositeTypes, ErpType.AX)]
public class CompositeTypeTask : ITaskAsync
{
...
public async Task<TaskStatus> Execute() => ...
}
Such attribute can be defined as follows:
[AttributeUsage(AttributeTargets.Class, Inherited = false, AllowMultiple = false)]
public sealed class TaskAttribute : Attribute
{
public TaskAttribute(string code, ErpType erp)
{
this.Code = code;
this.Erp = erp;
}
public string Code { get; }
public ErpType Erp { get; }
public (string Code, ErpType Erp) Key => (Code, Erp);
public static (string Code, ErpType) GetKey(Type type) =>
type.GetCustomAttribute<TaskAttribute>()?.Key
?? throw new InvalidOperationException($"{type} is not decorated with [Task] attribute.");
}
Instead of letting the TaskFinder
iterate through a collection, we could instead use a Dictionary, since it gives us O(1) performance. Here's an implementation:
public class TaskFinder : ITaskFinder
{
private readonly Dictionary<(string Code, ErpType Erp), InstanceProducer> producers;
public TaskFinder(
Dictionary<(string Code, ErpType Erp), InstanceProducer> producers)
{
this.producers = producers;
}
public (bool IsAsync, object Task) Get(string Code, ErpType Erp)
{
var key = (Code, Erp);
if (this.producers.TryGetValue(key, out InstanceProducer producer))
{
var task = producer.GetInstance();
return task is ITaskAsync ? (true, task) : (false, task);
}
throw new Exception($"Task {key} not found.");
}
}
TaskFinder
uses InstanceProducer
, which is a type from Simple Injector. An InstanceProducer
is what Simple Injector creates under the covers when you call Register
, but you can create them manually, which allows keyed registrations as you see above.
Since TaskFinder
now depends on Simple Injector, the implementation should be part of your Composition Root. You therefore might want to move this class.
Notice how this new TaskFinder
does not check whether there are multiple task instances, simply because a Dictionary
does not allow duplicate keys. Since we will create one TaskFinder
instance during startup, the application will simply not start when there are duplicate keys.
The last thing you need to do is change your Composition Root. The current registrations are the following:
container.RegisterCollection<ITask>(tasksAssemblies);
container.RegisterCollection<ITaskAsync>(tasksAssemblies);
container.RegisterSingleton<ITaskFinder, TaskFinder>();
We can change this to the following:
var taskRegistrations =
from type in container.GetTypesToRegister(typeof(ITask), tasksAssemblies)
select (TaskAttribute.GetKey(type), Lifestyle.Transient.CreateProducer(typeof(ITask), type, container));
var asyncTaskRegistrations =
from type in container.GetTypesToRegister(typeof(ITaskAsync), tasksAssemblies)
select (TaskAttribute.GetKey(type), Lifestyle.Transient.CreateProducer(typeof(ITaskAsync), type, container));
var finder = new TaskFinder(
taskRegistrations.Concat(asyncTaskRegistrations)
.ToDictionary(p => p.Item1, p => p.Item2));
container.RegisterSingleton<ITaskFinder>(finder);
Instead of calling RegisterCollection<ITask>
, you call GetTypesToRegister
, which returns a list of Type
instances. From that type a key-value pair is constructed, where TaskAttribute
gets the key for that type, and a Transient
InstanceProducer
is created as value.
The same is done for ITaskAsync
and both collections are packed together and converted to a Dictionary
and injected into the TaskFinder
. This TaskFinder
is registered in the container as Singleton
.
Of course, this change adds some complexity to the Composition Root, so you might want to postpone this change until you have proven that performance will become a problem. On the other hand, you might still want to use the TaskAttribute
, since it simplifies the task abstractions.
Oh! How didn't I think of attributes :dizzy_face:
I'll try this new approach and then I'll give you feedback.
And yes, performance will be a problem... But I was more concerned about finishing this before doing optimizations.
Now that the management board saw benefits on having this project I was given more budget to complete it.
Thank you! Feedback in a few hours.
But I was more concerned about finishing this before doing optimizations.
I think that's always the right approach.
Where can I find more info about InstanceProducer
?
Since TaskFinder now depends on Simple Injector, the implementation should be part of your Composition Root. You therefore might want to move this class.
Didn't understand the above comment.
Where would you move it?
I was thinking about letting it stay in the same folder, since there are many classes that use Container
and may be labeled as "part of the composition root" but still are in common folders.
I just made a small adjustment to the Task Registration:
var taskRegistrations =
from type in container.GetTypesToRegister(typeof(ITask), tasksAssemblies)
select (TaskAttribute.GetKey(type), Lifestyle.Transient.CreateProducer(typeof(ITask), type, container));
var asyncTaskRegistrations =
from type in container.GetTypesToRegister(typeof(ITaskAsync), tasksAssemblies)
select (TaskAttribute.GetKey(type), Lifestyle.Transient.CreateProducer(typeof(ITaskAsync), type, container));
var finder = new TaskFinder(
taskRegistrations.Concat(asyncTaskRegistrations)
.ToDictionary(p => p.Item1, p => p.Item2));
container.RegisterSingleton<ITaskFinder>(finder);
to
which I then call as
This InstanceProducer
seems neat.
I would like to learn more about the guts of SimpleInjector
, is there more docs other than simpleinjector.readthedocs.io?
Where can I find more info about InstanceProducer?
There are a few places that mention it, such as:
Didn't understand the above comment. Where would you move it?
Please understand the concept of the Composition Root, which is a:
unique location in an application where modules are composed together.
This means that:
A DI Container should only be referenced from the Composition Root. All other modules should have no reference to the container.
Hey!
While doing the registrations like this, is it possible to recreate the Tasks in runtime?
Since this is a singleton, it will get all the tasks when running the app the first time, but if for some reason one of the tasks disappear (changing one of the dll's), I want to disable it by clicking a button in my UI.
I want to run this code again:
However, if I've read the documentation correctly, it's not possible to change registrations after Verify()
.
Do you have any suggestions?
PS
By changing the dll, the application won't restart since the main dll's doesn't reference it. So I only have one option:
TaskFinder
and recreate it with this methodbut if for some reason one of the tasks disappear (changing one of the dll's)
You can't make any changes to any already loaded DLL in an AppDomain and neither can you remove an already loaded DLL. This is something that .NET does not support.
So if you are making any changes to DLLs, you already have to reload the AppDomain, which basically means restarting the application.
So unless I misunderstand what you are trying to achieve, this is not a problem with Simple Injector, but is inherent to the way .NET works. You will have to restart the application and at that point in time, you will be re-registering all your tasks (and other components) anyway.
So if you are making any changes to DLLs, you already have to reload the AppDomain, which basically means restarting the application.
This too applies for runtime loaded DLLs? I thought that it wouldn't restart since it's loaded on runtime.
From perspective of the CLR, there is no difference between a DLL that has been loaded at startup or a dynamically loaded DLL. In fact, DLLs that are loaded at startup, are just dynamically loaded by the JIT compiler.
So once a DLL is dynamically loaded (i.e. loaded by you), you can't unload it and can't replace it. You could however load an updated DLL from a different location, but the old DLL would still be loaded.
If you dynamically load a DLL, your AppDomain will not restart. You can load as many DLLs as you like, but you just can't unload or replace them.
Replacing them simply means you will have to restart the application. There's no way around this.
I see, I didn't think correctly about this. The current behavior of the CLR you explained is just enough for what I want.
If the app is restarted every time someone replaces the DLLs I don't need to workaround this.
Thank you a lot!
I don't know what kind of app you have, but:
It's an web app (IIS). So is IIS continuously looking for /bin changes?
So is IIS continuously looking for /bin changes?
Yep.
Hey,
I'm developing an web app to manage async Jobs/Tasks. I always create a new AsyncScope when launching a new Task.
Every time I get the IOperationContext, it will get the same instance, therefore I always have the OperationId and the cancellation token (ct).
However, I've the Entity Framework Context in the same situation (running in this scope).
There are some operations that I need to launch another AsyncScope since I want it to create a new Context but I want to preserve the old IOperationScope.
Therefore, is there any possibility to use both as Scoped (AsyncScoped) but one of them only uses the first AsyncScope it finds?
If there isn't anything built yet, I will try to create a CustomLifestyle and then I'll give feedback.