The reason I introduced the cache is that the core implementation of RuleDefinition fetches rules from the database. It's not about the memory footprint, it's that not all rule instantiation is cheap.
Peeking at the code, I see that we don't have a marker interface or method to indicate which rules are cacheable. I guess we should add a CacheableRule interface in the logic module, and have the code only cache these. It would really help us going forwards if there were a good unit test suite around heavy use of logic rules. I don't have the capacity to build this (neither time nor sufficient knowledge) but I'm happy to help coordinate. -Darius On Thursday, October 13, 2011, Anand, Vibha wrote: > Burke: One way to implement rule cache, which I do not find useful, and > still provide a thread safe interface is by keeping reference count on the > rule token / handle. From where I come, it’s important that OpenMRS as a > framework design around multithreaded / multitasking systems seriously. In > the same vein, I strongly agree and support that it is perfectly legal to > have instance variables in a rule class (translated or otherwise) as a > design decision. I do not see much usefulness for rule cache because I > think correct rule cache management is more expensive then the little > impact it might have on the memory footprint. I hope you will consider these > design issues in fixing this and future versions.**** > > *Vibha* > > * * > > ** ** > > *From:* [email protected] <javascript:_e({}, 'cvml', > '[email protected]');>[mailto: > [email protected] <javascript:_e({}, 'cvml', '[email protected]');>] *On > Behalf Of *Burke Mamlin > *Sent:* Thursday, October 13, 2011 12:50 PM > *To:* [email protected] <javascript:_e({}, 'cvml', > '[email protected]');> > *Subject:* Re: [OPENMRS-DEV] Major Logic Thread Safety Issue**** > > ** ** > > I'm not sure it's fair to expect rule authors to avoid any instance > variables. We either need to make new instances or – if that's a memory hog > – we could avoid needing to create a new instance every time by introducing > String > getState() and void setState(String) methods to the Rule interface.**** > > ** ** > > -Burke**** > > On Thu, Oct 13, 2011 at 6:10 PM, Ben Wolfe <[email protected]> wrote:**** > > I don't know of the implementation or reasoning here, but there is a lot > less memory used if its not instantiating new objects each time. I think we > should stick with that approach. We should either say that class level > variables are not allowed (like on controllers and servlets) or we say they > are allowed but all variables get cleared before a rule is used. > > Tammy, what was the fix in 1.5 for this? Is there a ticket? Was there a > regression test for it originally? > > Ben**** > > ** ** > > On Thu, Oct 13, 2011 at 5:46 PM, Tammy Dugan <[email protected]> wrote:*** > * > > It also corrupted 3 days worth of patient data across 4 clinics so needless > to say we were pretty unhappy that a problem we fixed in 1.5 came back in > 1.7 and corrupted patient data. > > Tammy**** > > > > On 10/13/2011 11:27 AM, McKee, Steven Jay wrote: **** > > Darius,**** > > **** > > CHICA encountered a major issue with Logic yesterday. We had information > get crossed between multiple patients, and we tracked it down to a thread > safety issue in Logic. Currently the Token Service’s “getRule(String)” > method (in OpenMRS 1.7) retrieves a rule from a rule provider and then > caches it. The next time the rule is asked for, it gets pulled directly > from the cache. Our Java rules translated by the Arden translator had class > level variables that were getting changed by concurrent threads, and we were > seeing incorrect data for patients.**** > > **** > > In OpenMRS 1.5 the logic service would cache the rule, but it would return > a new instantiation of the rule class so it was thread safe. Now in 1.7 + > the thread safety no longer exists because the same instance of the rule is > directly returned from the cache. **** > > **** > > This is a very high priority issue that needs to be resolved immediately.* > *** > > **** > > Thanks,**** > > **** > > Steve McKee**** > > Children’s Health Services Research**** > > Indiana University School of Medicine**** > > Phone: 317-278-9660**** > > Email: [email protected]**** > > **** > ------------------------------ > > Click here to unsubscribe from OpenMRS Developers' mailing list **** > > ** ** > > -- **** > > Tammy Dugan**** > > CHIRDL Technical Lead**** > > Children's Health Services Research**** > > IU School of Medicine**** > > ------------------------------ > > Click here to unsubscribe from OpenMRS Developers' mailing list **** > _________________________________________ To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to [email protected] with "SIGNOFF openmrs-devel-l" in the body (not the subject) of your e-mail. [mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l]
