-
-
Notifications
You must be signed in to change notification settings - Fork 837
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #1108 - Stop secondary invocation of OnActivating() handlers with decorated instance #1110
Conversation
@@ -102,9 +102,6 @@ public object Execute() | |||
_activationScope, | |||
resolveParameters); | |||
|
|||
if (instance != decoratorTarget) |
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.
Unless I'm missing something specific, you wouldn't actually want to raise the Activating event again for the same registration, but specify the decorated instance. CreateInstance() already raises Activating for the original, so this block that's removed is a secondary occurrence only happening with decorators in play.
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.
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.
It could be I'm just being overly cautious, but we should probably add some related tests to make sure there aren't any side effects here.
- I don't see any tests that ensure
Activating
only happens once, like, in general, without decorators. We should probably add some here. (Or, if you saw them, let me know where; I could be just missing them.) Be sure to include tests for a reflection-based registration (RegisterType<T>
), a lambda registration (Register(ctx => x)
), and a provided instance (RegisterInstance(obj)
). It might also be good to add the lambda and provided instance registration types to the decorator tests to make sure the right number of events are being raised there. - Update the existing tests you added to make sure Activating is also checked on the decorators - there should be an
Activating
for the instance being decorated and also one for each decorator.
Sorry if that's a little painful. It just appears that's a place where (as you've encountered) we probably need some additional testing to ensure we're not regressing or breaking something else by virtue of fixing this issue.
@tillig - I can definitely dig into the first one and shore up some additional tests, for the sake of adding robustness, but the second ask gives me pause, since that seems to contradict the fix that I added. Alternatively, are you asking that appropriate tests be added to ensure I don't know that decorating classes can have their own
where you might have an I'll do some more investigation, but wanted to make sure I understood the ask! |
Oh, no, you're right; I was thinking about the way the decorators automatically share the lifetime of the component to which they're attached and incorrectly extrapolated that there are also the same lifetime events for each decorator. Could have sworn that was a thing. Hmmm. Anyway, skip that second bullet. If we have some tests that just ensure we haven't incorrectly removed events or doubled events where they already were supposed to be, we'll be good. Sorry for the confusion. It's a hell of a day today. :) |
I'll investigate and reinforce tests as best I can in the next day or so.
No apologies needed, this isn't exactly an easy topic to visualize or
articulate! Plus it's Monday, which dooms us all ;)
|
…tions, based on lifetime configuration
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.
This is super awesome! Thanks!!!
Fixes #1108, specifically the most critical part, as it stops the casting exception from occurring or unexpectedly calling an OnActivating() delegate multiple times.
It also brings OnActivating() back into alignment with the other lifetime events when decorators are in play