j-easy / easy-rules

The simple, stupid rules engine for Java
https://github.com/j-easy/easy-rules/wiki
MIT License
4.87k stars 1.05k forks source link

ConcurrentModificationException while fetching RuleProxy.getRuleDescription #287

Closed shekharyadav200 closed 4 years ago

shekharyadav200 commented 4 years ago

we are getting below exception while running rulesEngine.fire(rules, facts) concurrently in multiple threads

"error.stack": " at java.base/java.util.TreeMap$PrivateEntryIterator.nextEntry(TreeMap.java:1208)\n at java.base/java.util.TreeMap$KeyIterator.next(TreeMap.java:1262)\n at org.jeasy.rules.core.RuleProxy.appendActionMethodsNames(RuleProxy.java:344)\n at org.jeasy.rules.core.RuleProxy.getRuleDescription(RuleProxy.java:325)\n at org.jeasy.rules.core.RuleProxy.invoke(RuleProxy.java:97)\n at com.sun.proxy.$Proxy98.getDescription(Unknown Source)\n at org.jeasy.rules.core.DefaultRulesEngine.log(DefaultRulesEngine.java:130)\n at org.jeasy.rules.core.DefaultRulesEngine.doFire(DefaultRulesEngine.java:75)\n at org.jeasy.rules.core.DefaultRulesEngine.fire(DefaultRulesEngine.java:65)\n at

fmbenhassine commented 4 years ago

Which version of easy rules do you use? Can you please provide a unit test that reproduces the issue? Thank you upfront.

shekharyadav200 commented 4 years ago

i am using 3.4.0 version of easy rule

I think this issue introduced in 3.4.0 and also there in latest version 4.0.0

suspected code for ConcurrentModificationException

private Set getActionMethodBeans() { if (this.actionMethods == null) { this.actionMethods = new TreeSet<>(); Method[] methods = getMethods(); for (Method method : methods) { if (method.isAnnotationPresent(Action.class)) { Action actionAnnotation = method.getAnnotation(Action.class); int order = actionAnnotation.order(); this.actionMethods.add(new ActionMethodOrderBean(method, order)); } } } return this.actionMethods; } private void appendActionMethodsNames(StringBuilder description) { Iterator iterator = getActionMethodBeans().iterator(); while (iterator.hasNext()) { description.append(iterator.next().getMethod().getName()); if (iterator.hasNext()) { description.append(","); } } }

when appendActionMethodsNames is invoked by multiple thread at time of rule engine start and value of Set not this.actionMethods not initialised

i will try to add test case which reproduce issue every time id possible and resolution in some time

fmbenhassine commented 4 years ago

Please provide a failing test before looking for a resolution, because this could be due to a misuse of the engine. Are you sharing facts between threads? While the rules and the rules engine can be shared between threads, facts on the other hand should not be shared (See FAQs). Here is a quick example:

public class Person {
    private int id;
        private String name;

    public Person(int id, String name) {
        this.id = id;
        this.name = name;
    }

    public int getId() {
        return id;
    }

    public void setId(int id) {
        this.id = id;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }
}

@Rule(name = "Hello rule", description = "Say hello to foos and bars")
public class HelloWorldRule {

    @Condition
    public boolean when(@Fact("person") Person person) {
        return person.getName().equals("foo") || person.getName().equals("bar");
    }

    @Action
    public void then(@Fact("person") Person person) {
        System.out.println("hello " + person.getName());
    }

}
import org.jeasy.rules.api.Facts;
import org.jeasy.rules.api.Rules;
import org.jeasy.rules.api.RulesEngine;
import org.jeasy.rules.core.DefaultRulesEngine;

public class Launcher {

    public static void main(String[] args) throws InterruptedException {

        // create facts (one instance per thread)
        Facts facts1 = new Facts();
        facts1.put("person", new Person(1, "foo"));
        Facts facts2 = new Facts();
        facts2.put("person", new Person(2, "bar"));
        Facts facts3 = new Facts();
        facts3.put("person", new Person(3, "foobar"));

        // create rules (can be shared between threads)
        Rules rules = new Rules();
        rules.register(new HelloWorldRule());

        // create a rules engine (can be shared between threads)
        RulesEngine rulesEngine = new DefaultRulesEngine();

        // quick and dirty, can use an executor service
        Thread t1 = new Thread(new Task(facts1, rules, rulesEngine));
        Thread t2 = new Thread(new Task(facts2, rules, rulesEngine));
        Thread t3 = new Thread(new Task(facts3, rules, rulesEngine));
        t1.start();
        t2.start();
        t3.start();

        t1.join();
        t2.join();
        t3.join();
    }

    static class Task implements Runnable {
        private Facts facts;
        private Rules rules;
        private RulesEngine rulesEngine;

        public Task(Facts facts, Rules rules, RulesEngine rulesEngine) {
            this.facts = facts;
            this.rules = rules;
            this.rulesEngine = rulesEngine;
        }

        @Override
        public void run() {
            rulesEngine.fire(rules, facts);
        }
    }
}

As you can see, facts are not shared between threads, whereas the rules and the rules engine are shared. The sample runs without any issues and prints:

hello bar
hello foo

as expected. Are you following this approach?

shekharyadav200 commented 4 years ago

YES i am using the same approach and not sharing the facts between threads

private Set getActionMethodBeans() { if (this.actionMethods == null) { this.actionMethods = new TreeSet<>(); Method[] methods = getMethods(); for (Method method : methods) { if (method.isAnnotationPresent(Action.class)) { Action actionAnnotation = method.getAnnotation(Action.class); int order = actionAnnotation.order(); this.actionMethods.add(new ActionMethodOrderBean(method, order)); } } } return this.actionMethods; } in This method if more multiple threads checking null condition at same time than all these threads will add the method name in ( this.actionMethods.add(new ActionMethodOrderBean(method, order));) treeSet

As this is tree set add operation also not thread safe as every thread can have different modCount in treeSet

while iterating in below method can throw java.util.ConcurrentModificationException

private void appendActionMethodsNames(StringBuilder description) { Iterator iterator = getActionMethodBeans().iterator(); while (iterator.hasNext()) { description.append(iterator.next().getMethod().getName()); if (iterator.hasNext()) { description.append(","); } } }

able to replicate using debug point in runtime environment

java.util.concurrent.ExecutionException: java.util.ConcurrentModificationException at java.base/java.util.concurrent.FutureTask.report(FutureTask.java:122) at java.base/java.util.concurrent.FutureTask.get(FutureTask.java:191) at test1.Launcher.lambda$2(Launcher.java:68) at java.base/java.util.ArrayList.forEach(ArrayList.java:1540) at test1.Launcher.main(Launcher.java:66) Caused by: java.util.ConcurrentModificationException at java.base/java.util.TreeMap$PrivateEntryIterator.nextEntry(TreeMap.java:1208) at java.base/java.util.TreeMap$KeyIterator.next(TreeMap.java:1262) at org.jeasy.rules.core.RuleProxy.appendActionMethodsNames(RuleProxy.java:344) at org.jeasy.rules.core.RuleProxy.getRuleDescription(RuleProxy.java:325) at org.jeasy.rules.core.RuleProxy.invoke(RuleProxy.java:97) at com.sun.proxy.$Proxy8.getDescription(Unknown Source) at org.jeasy.rules.core.DefaultRulesEngine.log(DefaultRulesEngine.java:130) at org.jeasy.rules.core.DefaultRulesEngine.doFire(DefaultRulesEngine.java:75) at org.jeasy.rules.core.DefaultRulesEngine.fire(DefaultRulesEngine.java:65) at test1.Launcher$Task.call(Launcher.java:127) at test1.Launcher$Task.call(Launcher.java:1) at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) at java.base/java.lang.Thread.run(Thread.java:835)

shekharyadav200 commented 4 years ago

public static void main(String[] args) throws InterruptedException {

    // create rules (can be shared between threads)
    Rules rules = new Rules();
    rules.register(new HelloWorldRule());

    // create a rules engine (can be shared between threads)
    RulesEngine rulesEngine = new DefaultRulesEngine();
    ThreadPoolExecutor executor = 
              (ThreadPoolExecutor) Executors.newFixedThreadPool(100); 
    List<Task> listTask =new ArrayList();

  List<Future<Boolean>> list =new ArrayList<Future<Boolean>>();
  IntStream.range(1, 1000).forEach(i->
  listTask.add(new Task(createFact( i), rules, rulesEngine))
  );
  listTask.forEach(task->
  list.add( executor.submit(task))
          );
  // to wait for  task complete
  list.forEach(future->{
    try {
        future.get();
    } catch (InterruptedException | ExecutionException e) {
        e.printStackTrace();
    }
});

}
 static Facts createFact(int i) {
     Facts facts3 = new Facts();
     facts3.put("person", new Person(3, UUID.randomUUID().toString()));
     return facts3;
 }
fmbenhassine commented 4 years ago

I'm still not able to reproduce the issue based on your example. Please format your comments and provide a minimal complete example that reproduces the issue in a reliable way.

Here is the minimal project I used: issue-287.zip