AutoActivate() should not hide a component's default service#1451
Conversation
…on the target component's first service being unique to that component
| 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.
nblumhardt
changed the title
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. |
tillig
left a comment
tillig
left a comment
There was a problem hiding this comment.
Looks good! I verified the changes on my local machine rather than fighting AppVeyor. It all builds and tests out correctly. I'll see if I can get this merged and get a 0.0.1 release out the door shortly.
|
Great - thanks! 👍 |
2 participants
Fixes #1450.