Dispose logger factory when disposing hosting engine #316
Conversation
| @@ -87,6 +88,7 @@ public virtual IDisposable Start() | |||
| _applicationLifetime.NotifyStopping(); | |||
| server.Dispose(); | |||
| _applicationLifetime.NotifyStopped(); | |||
| loggerFactory.Dispose(); | |||
There was a problem hiding this comment.
It feels odd to just dispose this service here. Not sure if there are any other services which need similar requirement.
There was a problem hiding this comment.
Agreed, this is weird. Isn't this what DI scopes are for? Would it make sense to create a scope on line 66 and dispose it here? Then any scoped services would be disposed.
There was a problem hiding this comment.
Hmm...I have seen DI scopes used in per-request kind of scenarios where as logger factory here is an application wide service. (thinking of it in a different way...the application start and shutdown itself is a scope and probably we should just dispose off all disposable application services as how DI scope would do).
There was a problem hiding this comment.
Consider in the test scenario where we start multiple TestHost/Servers in one process. HostingEngine is only a scoped subset of an actual application, so I think DI scopes are an appropriate concept here. Per-request scopes would be nested inside of the per-HostingEngine scopes.
There was a problem hiding this comment.
Why bother with the partial fix then? It's not urgent.
There was a problem hiding this comment.
The following PR depends on this
aspnet/Logging#162
There was a problem hiding this comment.
No, Hosting depends on Logging. That Logging PR is nice to have, but calling Dispose not a blocking issue for beta7.
Your partial fix here does not contribute towards the full fix, so don't bother.
kichalla
force-pushed
the
kiran/dispose-loggerfactory
branch
4 times, most recently
from
6353035 to
487afae
Compare
|
Closing this PR as it will be handled as part of fix for #322 |
@Tratcher This is related to the PR aspnet/Logging#162