ActivationRuleGroup and ConditionalRuleGroup not thread safe

[bas-jan]

First of all, thanks for a great framework.

While looking into using the CompositeRule implementations, I noticed that the ActivationRuleGroup and ConditionalRuleGroup are not thread-safe, since they have a state based on current Facts.

The ActivationRuleGroup has a field selectedRule which is set in the evaluate method, and is therefore in principle based on the current state of Facts.
The same holds for the ConditionalRuleGroup, where the field successfulEvaluations is also determined in evaluate.

In my case, I define my rules in a Spring context as singleton beans using @Component. That means that these CompositeRule classes cannot be used in such an application. This is especially problematic in a web application with multiple threads handling different concurrent HTTP requests.

[fmbenhassine]

First of all, thanks for a great framework.

Thank you!

Indeed, ActivationRuleGroup and ConditionalRuleGroup have some state which is not synchronized. This state is inevitable since it's used in different methods: evaluate and execute. Making these methods synchronized won't solve the problem because two threads could be calling evaluate and execute concurrently (on a probably different, incorrect state).

So I think we just need to document these classes as being non thread-safe. Do you agree? Otherwise, I'm open to other options if any.

BTW, have you tried to use spring's thread scope for your beans?

[bas-jan]

Thanks for your quick reply.

Yes, thread scope for the rule beans would work. But the way that I (like to) look at your framework is that it is composed of two main concepts:

• Logic, which is completely stateless and independent of the current data that is being processed. This logic is represented by Rule objects.
• Data, the state of the application, request, session, etc, on which the logic operatates. This data is represented by Fact objects.

So conceptually I expect the rules to be stateless and able to be instatiated as singletons. When something in the framework breaks with that expectation, I would indeed at least like to have that documented.

To make the ActivationRuleGroup and ConditionalRuleGroup stateless I can think of the following, all of which are probably not ideal:

• In these classes, do not separate the evaluation and execution, but handle everything in execute. This way you don't need to store the outcome of evaluate on the Rule instance. Of course, this breaks the whole evaluation-execution division and may have unwanted side effects depending on the way the RulesEngine works with these methods.
• Store any outcome of evaluate that you need for execute in the Facts. Maybe a special kind of Fact to store 'internal' state?
• Change the RulesEngine or implement a specific type of it to deal with these special kind of Rule instances that need to keep state.

[fmbenhassine]

You nailed it, this is explained in FAQ 6. All subclasses of CompositeRule have the inherited state for composing rules and their respective proxies, so it's not only about the state in these subclasses. Thank you for sharing those options, here are my thoughts:

In these classes, do not separate the evaluation and execution, but handle everything in execute.

As you mentioned, this breaks the whole evaluate-execute model and things like RuleEngine#check will break (as it is supposed to only call the evaluation method).

Store any outcome of evaluate that you need for execute in the Facts. Maybe a special kind of Fact to store 'internal' state?

While this might work, it will "pollute" the Facts (which are intended to store business facts) with technical facts. As a user, this means if I have a ConditionalRuleGroup in my rule set, and after firing the engine and trying to inspect facts, I will find a conditionalRule fact and a Set<Rule> successfulEvaluations fact in the Facts object. We can always find a way to tag them as internal or technical, but this still doesn't feel clean in my opinion.

Change the RulesEngine or implement a specific type of it to deal with these special kind of Rule instances that need to keep state.

The rules engine should not be aware of the Rule implementation to adapt its behaviour. This would be a mistake. The engine should work against the Rule interface.

Do you agree? I suggest to document those classes as being non thread-safe. wdyt?

[fmbenhassine]

I would indeed at least like to have that documented.

Hi, I couldn't find a clean way to make composite rules thread-safe, so I documented them as being thread-unsafe. You can try to use spring's thread scope, otherwise create a custom composite rule by using one of the options you mentioned.

Thank you for raising this anyway!

[bas-jan]

Thanks for clarifying and updating the FAQ.

[haotten]

I really like bas-jan's point of view around how he looks at your framework. Question for the team, what if you implemented a rule that instantiates (on creation) a RuleEngine internally, which could manage a group of rules. The facts passed to the "execute" method of this rule would simply fire the new rule engine. Would that make sense to create a thread safe "grouped" rule? You could set preferences to mimic your desired outcome.