Rule engine obj reloading #2396
Conversation
src/foam/nanos/ruler/RuleEngine.java
Outdated
| } | ||
| } | ||
|
|
||
| old = old.fclone(); |
There was a problem hiding this comment.
If you put this line in an else clause for the above 'if' you'll save unnecessary cloneing when you create an object on line 238.
There was a problem hiding this comment.
Oh yeah, you're right.
| @@ -161,11 +163,17 @@ private void asyncApplyRules(List<Rule> rules, FObject obj, FObject oldObj) { | |||
| if ( rule.getAsyncAction() != null | |||
| && rule.f(x, obj, oldObj) | |||
| ) { | |||
| // We assume the original object `obj` is stale when running after rules. | |||
There was a problem hiding this comment.
Can you describe a but that this fixes? If your action is dependent upon whether the object has been written in to the DAO yet or not, then I think we're setting ourselves up for race conditions and your rule would be better of running in an agency at a specific time. Or we should update the async logic to always fetch the latest object from the DAO, what state from the obj do you want that won't make it to nu ?
There was a problem hiding this comment.
We have before rules (sync and async) that can update the object by setting its properties. When the next async rule is run the object might not have been saved since saving happen on a separate CMD operation https://github.com/foam-framework/foam2/blob/master/src/foam/nanos/ruler/RulerDAO.js#L208.
As discussed with @kgrgreer, @jlhughes, and @nanoscott, we would have this kind of race condition because there is no synchronization inside the rule engine itself which is something needs to be addressed properly. On the other hand, we would need this temporary fix for our current release to not block the operation team from using IdentityMind integration.
I will be working on creating integration tests for IdentityMind then refactoring the rule engine to support object locking so that reloading would just be a simple find().
src/foam/nanos/ruler/RuleEngine.java
Outdated
| return obj; | ||
| } | ||
| } else { | ||
| old = old.fclone(); |
There was a problem hiding this comment.
@lchanmann I think Kevin mentioned this, it's not needed.
No description provided.