-
Notifications
You must be signed in to change notification settings - Fork 314
[Design] Reduce DI Allocations #289
Changes from 7 commits
6a0ad26
3f44164
a5746e4
572fc71
09e07f7
7194bcb
1b0d07b
9bef883
5560096
15b8df8
c64fb60
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ internal class ServiceProvider : IServiceProvider, IDisposable | |
|
||
private readonly Dictionary<IService, object> _resolvedServices = new Dictionary<IService, object>(); | ||
private ConcurrentBag<IDisposable> _disposables = new ConcurrentBag<IDisposable>(); | ||
private ConcurrentBag<IDisposable> _offlineDisposables; | ||
|
||
public ServiceProvider(IEnumerable<ServiceDescriptor> serviceDescriptors) | ||
{ | ||
|
@@ -143,16 +144,28 @@ internal IServiceCallSite GetResolveCallSite(IService service, ISet<Type> callSi | |
} | ||
} | ||
|
||
internal void Reset() | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like it was done to avoid a double-dispose if I suspect we're not too concerned about this scenario in practice. If there's any code you have operating on an SP while it's being disposed, you already have a bug. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would just empty any extras; or do nothing if there were none. I have a little concern as its a concurrentbag which suggests items can be added in parallel; or out of order and with delays. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In fact next comment is about the forced double dispose. |
||
if (_offlineDisposables == null) | ||
{ | ||
throw new InvalidOperationException("ServiceProvider reused before being disposed"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a resx in this project? Programmatic names like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done? |
||
} | ||
_disposables = _offlineDisposables; | ||
_offlineDisposables = null; | ||
} | ||
|
||
public void Dispose() | ||
{ | ||
var disposables = Interlocked.Exchange(ref _disposables, null); | ||
|
||
if (disposables != null) | ||
{ | ||
foreach (var disposable in disposables) | ||
IDisposable disposable; | ||
while (disposables.TryTake(out disposable)) | ||
{ | ||
disposable.Dispose(); | ||
} | ||
_resolvedServices.Clear(); | ||
_offlineDisposables = disposables; | ||
} | ||
} | ||
|
||
|
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.
Why internal here? It would also be helpful to capture your explanation of this in comments.
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.
Not sure what to write... Its a method that is explicitly bonded to the pooling of ServiceScopeFactory so if the class were made public rather than internal for whatever reason the exposed api would be for it to function as a standalone class (which is why I left public constructor in); whereas the "internals" only work as a class pair.
Not hugely fussed on visibility though, can make public
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.
I would leave it internal