Fixed a bug in how global bindings were tracked/applied. There were several problems here (which ultimately affected the 3.3.0 branch), but mainly global bindings were being re-applied to the ScriptContext given to eval() and thus making them both engine and global bindings which then led to local variables getting assigned to the global scope. Also, getGlobalBindings() on GremlinExecutor is no longer necessary and is deprecated as the global bindings should be manipulated directly on the GremlinScriptEngine. CTR
Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/42dc0c7d Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/42dc0c7d Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/42dc0c7d Branch: refs/heads/TINKERPOP-1625 Commit: 42dc0c7de0ad26e3602faebf0c10ee8795bee971 Parents: a052b1f Author: Stephen Mallette <sp...@genoprime.com> Authored: Thu Mar 23 02:51:08 2017 -0400 Committer: Stephen Mallette <sp...@genoprime.com> Committed: Thu Mar 23 02:51:08 2017 -0400 ---------------------------------------------------------------------- .../DefaultGremlinScriptEngineManager.java | 7 +- .../gremlin/groovy/engine/GremlinExecutor.java | 5 + .../groovy/engine/GremlinExecutorTest.java | 109 ++++++++++++++++++- 3 files changed, 116 insertions(+), 5 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/42dc0c7d/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/DefaultGremlinScriptEngineManager.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/DefaultGremlinScriptEngineManager.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/DefaultGremlinScriptEngineManager.java index c8b77e6..09862be 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/DefaultGremlinScriptEngineManager.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/DefaultGremlinScriptEngineManager.java @@ -478,8 +478,11 @@ public class DefaultGremlinScriptEngineManager implements GremlinScriptEngineMan // registered by certain GremlinScriptEngine instances (pretty much talking about gremlin-groovy here) // where type checking is made important. this may not be a good generic way to handled this in the // long run, but for now we only have two GremlinScriptEngines to be concerned about so thus far it - // presents no real pains - final Object initializedBindings = engine.eval(initScript, getBindings()); + // presents no real pains. global bindings are applied automatically to the context via + // AbstractScriptEngine.getScriptContext() - passing them again here to eval() will just make the + // global bindings behave as engine bindings and then you get weird things happening (like local vars + // becoming global). + final Object initializedBindings = engine.eval(initScript); if (initializedBindings != null && initializedBindings instanceof Map) ((Map<String,Object>) initializedBindings).forEach((k,v) -> put(k,v)); } catch (Exception ex) { http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/42dc0c7d/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutor.java ---------------------------------------------------------------------- diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutor.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutor.java index 8fdb86e..8802793 100644 --- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutor.java +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutor.java @@ -388,6 +388,11 @@ public class GremlinExecutor implements AutoCloseable { return scheduledExecutorService; } + /** + * @deprecated As of release 3.2.5, replaced by {@link #getScriptEngineManager()} to add global scoped bindings + * directly to that object. + */ + @Deprecated public Bindings getGlobalBindings() { return globalBindings; } http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/42dc0c7d/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutorTest.java ---------------------------------------------------------------------- diff --git a/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutorTest.java b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutorTest.java index a56f805..8aed8a6 100644 --- a/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutorTest.java +++ b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutorTest.java @@ -23,6 +23,8 @@ import org.apache.tinkerpop.gremlin.TestHelper; import org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.ThreadInterruptCustomizerProvider; import org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.TimedInterruptCustomizerProvider; import org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.TimedInterruptTimeoutException; +import org.apache.tinkerpop.gremlin.jsr223.ImportGremlinPlugin; +import org.apache.tinkerpop.gremlin.jsr223.ScriptFileGremlinPlugin; import org.javatuples.Pair; import org.junit.Test; @@ -70,6 +72,22 @@ public class GremlinExecutorTest { public static Map<String, String> PATHS = new HashMap<>(); private final BasicThreadFactory testingThreadFactory = new BasicThreadFactory.Builder().namingPattern("test-gremlin-executor-%d").build(); + /** + * Temporary "useless" plugin definition to force GremlinExecutor to use GremlinScriptEngineManager - will be + * removed when the old functionality of ScriptEngines is removed. + */ + private final Map<String, Map<String,Object>> triggerPlugin = new HashMap<String, Map<String,Object>>() {{ + put(ImportGremlinPlugin.class.getName(), new HashMap<String,Object>() {{ + put("classImports", Collections.singletonList("java.lang.Math")); + }}); + }}; + + private final Map<String, Map<String,Object>> scriptFilePlugin = new HashMap<String, Map<String,Object>>() {{ + put(ScriptFileGremlinPlugin.class.getName(), new HashMap<String,Object>() {{ + put("files", Collections.singletonList(PATHS.get("GremlinExecutorInit.groovy"))); + }}); + }}; + static { try { final List<String> groovyScriptResources = Collections.singletonList("GremlinExecutorInit.groovy"); @@ -213,7 +231,7 @@ public class GremlinExecutorTest { } @Test - public void shouldGetGlobalBindings() throws Exception { + public void shouldGetGlobalBindingsDeprecated() throws Exception { final Bindings b = new SimpleBindings(); final Object bound = new Object(); b.put("x", bound); @@ -223,6 +241,18 @@ public class GremlinExecutorTest { } @Test + public void shouldGetGlobalBindings() throws Exception { + final Bindings b = new SimpleBindings(); + final Object bound = new Object(); + b.put("x", bound); + final GremlinExecutor gremlinExecutor = GremlinExecutor.build(). + addPlugins("gremlin-groovy", triggerPlugin). + globalBindings(b).create(); + assertEquals(bound, gremlinExecutor.getScriptEngineManager().getBindings().get("x")); + gremlinExecutor.close(); + } + + @Test public void shouldEvalScriptWithGlobalAndLocalBindings() throws Exception { final Bindings g = new SimpleBindings(); g.put("x", 1); @@ -509,7 +539,7 @@ public class GremlinExecutorTest { } @Test - public void shouldInitializeWithScriptAndMakeGlobalBinding() throws Exception { + public void shouldInitializeWithScriptAndMakeGlobalBindingDeprecated() throws Exception { final GremlinExecutor gremlinExecutor = GremlinExecutor.build() .addEngineSettings("gremlin-groovy", Collections.emptyList(), @@ -528,6 +558,21 @@ public class GremlinExecutorTest { } @Test + public void shouldInitializeWithScriptAndMakeGlobalBinding() throws Exception { + final GremlinExecutor gremlinExecutor = GremlinExecutor.build() + .addPlugins("gremlin-groovy", scriptFilePlugin) + .create(); + + assertEquals(2, gremlinExecutor.eval("add(1,1)").get()); + assertThat(gremlinExecutor.getScriptEngineManager().getBindings().keySet(), contains("name")); + assertThat(gremlinExecutor.getScriptEngineManager().getBindings().keySet(), not(hasItem("someSet"))); + + assertEquals("stephen", gremlinExecutor.getScriptEngineManager().getBindings().get("name")); + + gremlinExecutor.close(); + } + + @Test public void shouldContinueToEvalScriptsEvenWithTimedInterrupt() throws Exception { final Map<String,List<Object>> compilerCustomizerConfig = new HashMap<>(); final List<Object> args = new ArrayList<>(); @@ -694,7 +739,7 @@ public class GremlinExecutorTest { } @Test - public void shouldAllowConcurrentModificationOfGlobals() throws Exception { + public void shouldAllowConcurrentModificationOfGlobalsDeprecated() throws Exception { // this test simulates a scenario that likely shouldn't happen - where globals are modified by multiple // threads. globals are created in a synchronized fashion typically but it's possible that someone // could do something like this and this test validate that concurrency exceptions don't occur as a @@ -748,4 +793,62 @@ public class GremlinExecutorTest { assertThat(t.getValue1().get(3).intValue(), greaterThan(-1)); }); } + + @Test + public void shouldAllowConcurrentModificationOfGlobals() throws Exception { + // this test simulates a scenario that likely shouldn't happen - where globals are modified by multiple + // threads. globals are created in a synchronized fashion typically but it's possible that someone + // could do something like this and this test validate that concurrency exceptions don't occur as a + // result + final ExecutorService service = Executors.newFixedThreadPool(8, testingThreadFactory); + final Bindings globals = new SimpleBindings(); + globals.put("g", -1); + final GremlinExecutor gremlinExecutor = GremlinExecutor.build() + .addPlugins("gremlin-groovy", triggerPlugin) + .globalBindings(globals).create(); + + final AtomicBoolean failed = new AtomicBoolean(false); + final int max = 512; + final List<Pair<Integer, List<Integer>>> futures = Collections.synchronizedList(new ArrayList<>(max)); + IntStream.range(0, max).forEach(i -> { + final int yValue = i * 2; + final Bindings b = new SimpleBindings(); + b.put("x", i); + b.put("y", yValue); + final int zValue = i * -1; + + final String script = "z=" + zValue + ";[x,y,z,g]"; + try { + service.submit(() -> { + try { + // modify the global in a separate thread + gremlinExecutor.getScriptEngineManager().put("g", i); + gremlinExecutor.getScriptEngineManager().put(Integer.toString(i), i); + gremlinExecutor.getScriptEngineManager().getBindings().keySet().stream().filter(s -> i % 2 == 0 && !s.equals("g")).findFirst().ifPresent(globals::remove); + final List<Integer> result = (List<Integer>) gremlinExecutor.eval(script, b).get(); + futures.add(Pair.with(i, result)); + } catch (Exception ex) { + failed.set(true); + } + }); + } catch (Exception ex) { + throw new RuntimeException(ex); + } + }); + + service.shutdown(); + assertThat(service.awaitTermination(60000, TimeUnit.MILLISECONDS), is(true)); + + // likely a concurrency exception if it occurs - and if it does then we've messed up because that's what this + // test is partially designed to protected against. + assertThat(failed.get(), is(false)); + + assertEquals(max, futures.size()); + futures.forEach(t -> { + assertEquals(t.getValue0(), t.getValue1().get(0)); + assertEquals(t.getValue0() * 2, t.getValue1().get(1).intValue()); + assertEquals(t.getValue0() * -1, t.getValue1().get(2).intValue()); + assertThat(t.getValue1().get(3).intValue(), greaterThan(-1)); + }); + } }
