Skip to content

Commit

Permalink
was able to avoid deep clones - it was a mistake #1685
Browse files Browse the repository at this point in the history
  • Loading branch information
@ptrthomas
ptrthomas committed Jul 20, 2021
1 parent 6d8aa7c commit 77408d8
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 100 deletions.
7 changes: 4 additions & 3 deletions karate-core/src/main/java/com/intuit/karate/core/ScenarioBridge.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,9 @@ private static Object callSingleResult(ScenarioEngine engine, Object o) throws E
engine.logger.warn("callSingle() cached result is an exception");
throw (Exception) o;
}
// if we don't deep clone, an attach operation would update the tree within the cached value
// if we don't clone, an attach operation would update the tree within the cached value
// causing future cache hit + attach attempts to fail !
o = engine.recurseAndAttachAndDeepClone(o);
o = engine.recurseAndAttachAndShallowClone(o);
return JsValue.fromJava(o);
}

Expand Down Expand Up @@ -245,7 +245,8 @@ public Object callSingle(String fileName, Value arg) throws Exception {
engine.logger.warn("callSingleCache write failed, not json-like: {}", resultVar);
}
}
result = engine.recurseAndDetachAndDeepClone(resultVar.getValue());
// functions have to be detached so that they can be re-hydrated in another js context
result = engine.recurseAndDetachAndShallowClone(resultVar.getValue());
}
CACHE.put(fileName, result);
engine.logger.info("<< lock released, cached callSingle: {}", fileName);
Expand Down
173 changes: 76 additions & 97 deletions karate-core/src/main/java/com/intuit/karate/core/ScenarioEngine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1003,20 +1003,20 @@ public void init() { // not in constructor because it has to be on Runnable.run(
// even hidden variables may need pre-processing
// for e.g. the __arg may contain functions that originated in a different js context
Object o = recurseAndAttach(k, v, seen);
JS.put(k, o == null ? v : o);
JS.put(k, o == null ? v : o); // attach returns null if "not dirty"
});
vars.forEach((k, v) -> {
// re-hydrate any functions from caller or background
Object o = recurseAndAttach(k, v.getValue(), seen);
// note that we don't update the vars !
// if we do, any "bad" out-of-context values will crash the constructor of Variable
// it is possible the vars are detached / re-used later, so we kind of defer the inevitable
JS.put(k, o == null ? v.getValue() : o);
JS.put(k, o == null ? v.getValue() : o); // attach returns null if "not dirty"
});
if (runtime.caller.arg != null && runtime.caller.arg.isMap()) {
// add the call arg as separate "over ride" variables
Map<String, Object> arg = runtime.caller.arg.getValue();
recurseAndAttach("", arg, seen);
recurseAndAttach("", arg, seen); // since arg is a map, it will not be cloned
arg.forEach((k, v) -> {
vars.put(k, new Variable(v));
JS.put(k, v);
Expand Down Expand Up @@ -1046,11 +1046,27 @@ protected Map<String, Variable> detachVariables() {
Set<Object> seen = Collections.newSetFromMap(new IdentityHashMap());
Map<String, Variable> detached = new HashMap(vars.size());
vars.forEach((k, v) -> {
Object o = recurseAndDetachAndDeepClone(k, v.getValue(), seen);
Object o = recurseAndDetachAndShallowClone(k, v.getValue(), seen);
detached.put(k, new Variable(o));
});
return detached;
}

// callSingle
protected Object recurseAndAttachAndShallowClone(Object o) {
return recurseAndAttachAndShallowClone(o, Collections.newSetFromMap(new IdentityHashMap()));
}

// callonce
protected Object recurseAndAttachAndShallowClone(Object o, Set<Object> seen) {
if (o instanceof List) {
o = new ArrayList((List) o);
} else if (o instanceof Map) {
o = new LinkedHashMap((Map) o);
}
Object result = recurseAndAttach("", o, seen);
return result == null ? o : result;
}

private Object recurseAndAttach(String name, Object o, Set<Object> seen) {
if (o instanceof Value) {
Expand Down Expand Up @@ -1087,101 +1103,57 @@ private Object recurseAndAttach(String name, Object o, Set<Object> seen) {
if (seen.add(o)) {
List list = (List) o;
int count = list.size();
for (int i = 0; i < count; i++) {
Object child = list.get(i);
Object childResult = recurseAndAttach(name + "[" + i + "]", child, seen);
if (childResult != null) {
try {
try {
for (int i = 0; i < count; i++) {
Object child = list.get(i);
Object childResult = recurseAndAttach(name + "[" + i + "]", child, seen);
if (childResult != null) {
list.set(i, childResult);
} catch (Exception e) {
logger.warn("immutable list item: {}", name + "[" + i + "]");
}
}
} catch (Exception e) {
logger.warn("attach - immutable list: {}", name);
}
}
return null;
} else if (o instanceof Map) {
if (seen.add(o)) {
Map<String, Object> map = (Map) o;
map.forEach((k, v) -> {
Object childResult = recurseAndAttach(name + "." + k, v, seen);
if (childResult != null) {
try {
try {
map.forEach((k, v) -> {
Object childResult = recurseAndAttach(name + "." + k, v, seen);
if (childResult != null) {
map.put(k, childResult);
} catch (Exception e) {
logger.warn("immutable map entry: {}", name + "." + k);
}
}
});
});
} catch (Exception e) {
logger.warn("attach - immutable map: {}", name);
}
}
return null;
} else {
return null;
}
}

// called only by result processing of callonce / callSingle
protected Object recurseAndAttachAndDeepClone(Object o) {
Set<Object> seen = Collections.newSetFromMap(new IdentityHashMap());
return recurseAndAttachAndDeepClone("", o, seen);
protected Object recurseAndDetachAndShallowClone(Object o) {
return recurseAndDetachAndShallowClone("", o, Collections.newSetFromMap(new IdentityHashMap()));
}

private Object recurseAndAttachAndDeepClone(String name, Object o, Set<Object> seen) {
if (o instanceof Value) {
try {
return Value.asValue(o);
} catch (Exception e) {
logger.warn("[*** attach deep ***] ignoring non-json value: '{}' - {}", name, e.getMessage());
return null;
}
}
if (o instanceof Class) {
Class clazz = (Class) o;
return JS.evalForValue("Java.type('" + clazz.getCanonicalName() + "')");
} else if (o instanceof JsFunction) {
JsFunction jf = (JsFunction) o;
try {
return attachSource(jf.source);
} catch (Exception e) {
logger.warn("[*** attach deep ***] ignoring js-function: '{}' - {}", name, e.getMessage());
return null;
}
} else if (o instanceof List) {
if (seen.add(o)) {
List list = (List) o;
int count = list.size();
List copy = new ArrayList(count);
for (int i = 0; i < count; i++) {
Object child = list.get(i);
copy.add(recurseAndAttachAndDeepClone(name + "[" + i + "]", child, seen));
}
return copy;
} else {
return o;
}
// callonce, callSingle and detachVariables()
private Object recurseAndDetachAndShallowClone(String name, Object o, Set<Object> seen) {
if (o instanceof List) {
o = new ArrayList((List) o);
} else if (o instanceof Map) {
if (seen.add(o)) {
Map<String, Object> map = (Map) o;
Map<String, Object> copy = new LinkedHashMap(map.size());
map.forEach((k, v) -> copy.put(k, recurseAndAttachAndDeepClone(name + "." + k, v, seen)));
return copy;
} else {
return o;
}
} else {
return o;
o = new LinkedHashMap((Map) o);
}
Object result = recurseAndDetach(name, o, seen);
return result == null ? o : result;
}

// only for callonce and callSingle
protected Object recurseAndDetachAndDeepClone(Object o) {
Set<Object> seen = Collections.newSetFromMap(new IdentityHashMap());
return recurseAndDetachAndDeepClone("", o, seen);
}

private Object recurseAndDetachAndDeepClone(String name, Object o, Set<Object> seen) {
private Object recurseAndDetach(String name, Object o, Set<Object> seen) {
if (o instanceof Value) {
Value value = Value.asValue(o);
Value value = (Value) o;
try {
if (value.canExecute()) {
if (value.isMetaObject()) { // js function
Expand All @@ -1193,34 +1165,41 @@ private Object recurseAndDetachAndDeepClone(String name, Object o, Set<Object> s
return value.asHostObject();
}
} catch (Exception e) {
logger.warn("[*** detach deep ***] ignoring non-json value in callonce / callSingle: '{}' - {}", name, e.getMessage());
return value; // re-attempt on attach
logger.warn("[*** detach ***] ignoring non-json value: '{}' - {}", name, e.getMessage());
}
}
if (o instanceof List) {
if (seen.add(o)) {
List list = (List) o;
int count = list.size();
List copy = new ArrayList(list.size());
return null;
} else if (o instanceof List) {
List list = (List) o;
int count = list.size();
try {
for (int i = 0; i < count; i++) {
Object child = list.get(i);
copy.add(recurseAndDetachAndDeepClone(name + "[" + i + "]", child, seen));
Object childResult = recurseAndDetach(name + "[" + i + "]", child, seen);
if (childResult != null) {
list.set(i, childResult);
}
}
return copy;
} else {
return o;
} catch (Exception e) {
logger.warn("detach - immutable list: {}", name);
}
return null;
} else if (o instanceof Map) {
if (seen.add(o)) {
Map<String, Object> map = (Map) o;
Map<String, Object> copy = new LinkedHashMap(map.size());
map.forEach((k, v) -> copy.put(k, recurseAndDetachAndDeepClone(name + "." + k, v, seen)));
return copy;
} else {
return o;
try {
map.forEach((k, v) -> {
Object childResult = recurseAndDetach(name + "." + k, v, seen);
if (childResult != null) {
map.put(k, childResult);
}
});
} catch (Exception e) {
logger.warn("detach - immutable map: {}", name);
}
}
return null;
} else {
return o;
return null;
}
}

Expand Down Expand Up @@ -1955,12 +1934,12 @@ public Variable call(boolean callOnce, String exp, boolean sharedScope) {

private Variable callOnceResult(ScenarioCall.Result result, boolean sharedScope) {
if (sharedScope) { // if shared scope
vars.clear(); // clean slate
// deep-clone so that subsequent steps don't modify data / references being passed around
vars.clear(); // clean slate
if (result.vars != null) {
Set<Object> seen = Collections.newSetFromMap(new IdentityHashMap());
result.vars.forEach((k, v) -> {
Object o = recurseAndAttachAndDeepClone(k, v.getValue(), seen);
// clone maps and lists so that subsequent steps don't modify data / references being passed around
Object o = recurseAndAttachAndShallowClone(v.getValue(), seen);
try {
vars.put(k, new Variable(o));
} catch (Exception e) {
Expand All @@ -1977,7 +1956,7 @@ private Variable callOnceResult(ScenarioCall.Result result, boolean sharedScope)
// note that shared scope means a return value is meaningless
} else {
// deep-clone for the same reasons mentioned above
Object resultValue = recurseAndAttachAndDeepClone(result.value.getValue());
Object resultValue = recurseAndAttachAndShallowClone(result.value.getValue());
return new Variable(resultValue);
}
}
Expand Down Expand Up @@ -2012,7 +1991,7 @@ private Variable callOnce(String cacheKey, Variable called, Variable arg, boolea
Map<String, Variable> clonedVars = called.isFeature() && sharedScope ? detachVariables() : null;
Config clonedConfig = new Config(config);
clonedConfig.detach();
Object resultObject = recurseAndDetachAndDeepClone(resultValue.getValue());
Object resultObject = recurseAndDetachAndShallowClone(resultValue.getValue());
result = new ScenarioCall.Result(new Variable(resultObject), clonedConfig, clonedVars);
CACHE.put(cacheKey, result);
logger.info("<< lock released, cached callonce: {}", cacheKey);
Expand Down

0 comments on commit 77408d8

Please sign in to comment.