AutoActivate() should not hide a component's default service
#1451
Conversation
…on the target component's first service being unique to that component
| @@ -43,12 +43,15 @@ public IEnumerable<Service> Services | |||
| { | |||
| get | |||
| { | |||
| if (_defaultServiceOverridden) | |||
| if (!_defaultServiceOverridden && !_services.Contains(_defaultService)) | |||
There was a problem hiding this comment.
Changed the declared type of _services above so that it's clear this is not an O(N) operation.
AutoActivate() should not hide a component's default service
|
Not sure why AppVeyor is reporting this as non-mergeable; is that just a policy-driven thing, or is there some kind of conflict lurking? (Hoping I targeted the right branch?) |
| // https://github.com/autofac/Autofac/issues/1102 | ||
| // Single instance registrations with any custom activation phases (i.e. activation handlers) need to be auto-activated, | ||
| // so that other behavior (such as OnRelease) that expects 'normal' object lifetime behavior works as expected. | ||
| rb.RegistrationData.AddService(new AutoActivateService()); |
There was a problem hiding this comment.
This simplification wasn't possible prior to the other fix, as it would have broken default service logic.
| // so that other behavior (such as OnRelease) that expects 'normal' object lifetime behavior works as expected. | ||
| if (rb.ResolvePipeline.Middleware.Any(s => s.Phase == PipelinePhase.Activation)) | ||
| { | ||
| var autoStartService = rb.RegistrationData.Services.First(); |
There was a problem hiding this comment.
The bug here is due to the possibility that there are multiple registrations for this service; resolving it won't guarantee that we end up with the same component (and in fact, some unrelated random thing could end up unexpectedly activated :-))
|
I'm not sure why it's saying that, but I'm on holiday until next week so I won't be able to figure it out until then. It... should just build. 🤷♂️ |
|
I've poked around on this trying to figure out why AppVeyor won't merge this properly and I've not found anything helpful. I'll just test it locally and call it good. I think the changes are fine. I want to move to GitHub Actions anyway, so it's not worth fighting too hard with the AppVeyor issue. |
|
Great - thanks! 👍 |
Fixes #1450.