-
Notifications
You must be signed in to change notification settings - Fork 220
fix: cover informer automatic start case #662
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
Conversation
* automatically started, what would cause a NullPointerException here, since an event might | ||
* be received between creation and registration. | ||
*/ | ||
if (eventHandler != null) { |
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 think this need some sort of protection as there may be a race conditions between the event being fired and the eventHandler
being set in the AbstractEventSource
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.
Normally it is LifecycleAware
, and we "run
" the informer only on start
. But there are some cases where the informer is run automtically when created in fabric8 client "Informable
" way, (what we want, like it is passed as a param). Will address this in an issue for fabric8 client.
For now this is an ugly workaround :/
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.
Yes but my understanding is that propagateEvent
may run as per the invocation of the constructor so while the propagateEvent
in invoked by an informer thread another thread my set the eventHandler
.
Since the eventHandler
is neither volatile nor protected by a lock then the two thread my see a different value for eventHandler
, right ? Also, since there is a list of events, some events may get propagated and some other not.
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.
ahh, good point, changed it to volatile
.
But I don't understand the second point:
Also, since there is a list of events, some events may get propagated and some other not.
All the events are propagated from a list from one thread, so it's either sees the variable set or not, or?
Or the thread can see the actual reference from certain point of time...? This is what you mean?
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.
assuming that esourceToUIDs.apply(object)
produces 3 elements, and the eventHandler
is set once the uids.forEach
is processing the 2nd element, then the 3rd UID will trigger a reconcile, the first two don't.
It may be ok but I don't know.
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.
The thing is with volatile (now set, thx) and LifecycleAware
all should be covered, if the Informer is created and did not run. Now it can happen that if created in a certain way Informer.run method called automatically in fabric8 - will address this in fabric8 client and a subsequent issue here.
For now this workaround should mitigate this issues, there can be missed events on startup, but since this is event source and not a the main custom resource event source, it's not that big deal IMHO. Hopefully we can address this before we release v2. But actually will be just a docs from our side to not create an Informer event source when set as paramter. (Or check if the passed event source is running, in case yes log a warning)
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.
Ok it's doable this way:
7eb3aa7
Added warn log message if user passes a running informer.
No description provided.