Skip to content
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.

Sign up for GitHub

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

Rule engine obj reloading #2396

Merged
merged 6 commits into from
Jul 15, 2019
Merged

Rule engine obj reloading #2396

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b2442dd
Reload obj before running async action
lchanmann Jul 12, 2019
88e2f92
Fix context when retrying async action
lchanmann Jul 12, 2019
6b92d7f
Refactor reloadObject to take reloaded FObject instead of X and updat…
lchanmann Jul 13, 2019
a07483c
Return reloaded object instead of re-assignment
lchanmann Jul 13, 2019
7ad0520
Prevent unnecessary cloneing old object
lchanmann Jul 14, 2019
2fae054
Remove unnecessary cloneing
lchanmann Jul 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 70 additions & 4 deletions src/foam/nanos/ruler/RuleEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

import foam.core.*;
import foam.dao.DAO;
import foam.nanos.auth.LastModifiedAware;
import foam.nanos.auth.LastModifiedByAware;
import foam.nanos.logger.Logger;
import foam.nanos.pm.PM;
import foam.nanos.pool.FixedThreadPool;
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

// For that, greedy mode is used for object reload. For before rules,
// object reload uses non-greedy mode so that changes on the original
// object will be copied over to the reloaded object.
FObject nu = getDelegate().find_(x, obj);
reloadObject(obj, oldObj, nu, rule.getAfter());
jlhughes marked this conversation as resolved.
Show resolved Hide resolved
try {
rule.asyncApply(x, obj, oldObj, RuleEngine.this);
saveHistory(rule, obj);
rule.asyncApply(x, nu, oldObj, RuleEngine.this);
saveHistory(rule, nu);
} catch (Exception ex) {
retryAsyncApply(x, rule, obj, oldObj);
retryAsyncApply(x, rule, nu, oldObj);
}
}
}
Expand All @@ -174,7 +182,7 @@ private void asyncApplyRules(List<Rule> rules, FObject obj, FObject oldObj) {

private void retryAsyncApply(X x, Rule rule, FObject obj, FObject oldObj) {
new RetryManager().submit(x, x1 -> {
rule.asyncApply(getX(), obj, oldObj, RuleEngine.this);
rule.asyncApply(x, obj, oldObj, RuleEngine.this);
saveHistory(rule, obj);
});
}
Expand Down Expand Up @@ -203,4 +211,62 @@ record = new RuleHistory.Builder(getX())
savedRuleHistory_.put(rule.getId(),
(RuleHistory) ruleHistoryDAO_.put(record).fclone());
}

/**
* Reloads object when running async action since the original object `obj`
* might be stale at the time of execution.
*
* If reloaded object `nu` hasn't changed (reloaded object equals to old
* object) then return the original object as the reloaded object.
*
* If reloaded object has changed and already saved to DAO (reloaded object
* equals to original object) then return the original object.
*
* Otherwise, return the reloaded object. If greedy flag is set to true then
* also copy changes from the original object to the reloaded object.
*
* @param obj - Original object
* @param oldObj - Old object
* @param nu - Reloaded object
* @param greedy - Flag to set greedy mode
*/
private void reloadObject(FObject obj, FObject oldObj, FObject nu, boolean greedy) {
FObject old = oldObj;
if ( old == null ) {
try {
old = obj.getClass().newInstance();
} catch (Exception e) {
// Object instantiation should not fail but if it does fail then use the
// original object as the reloaded object.
nu = obj;
}
}

old = old.fclone();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, you're right.

FObject cloned = obj.fclone();

// Update lastModified and lastModifiedBy of old and cloned objects so that
// equality comparisons would not be skewed by these properties.
if ( obj instanceof LastModifiedAware ) {
Date lastModified = ((LastModifiedAware) nu).getLastModified();
((LastModifiedAware) cloned).setLastModified(lastModified);
((LastModifiedAware) old).setLastModified(lastModified);
}
if ( obj instanceof LastModifiedByAware ) {
long lastModifiedBy = ((LastModifiedByAware) nu).getLastModifiedBy();
((LastModifiedByAware) cloned).setLastModifiedBy(lastModifiedBy);
((LastModifiedByAware) old).setLastModifiedBy(lastModifiedBy);
}

// Use the original object as the reloaded object if nu == old or nu == obj.
if ( nu.equals(old) || nu.equals(cloned) ) {
nu = obj;
}

// For greedy mode, use the reloaded object (nu) as is. Otherwise, override
// the reloaded object with the changes from the original object.
if ( greedy ) {
nu.copyFrom(obj);
}
}
}