Hi, This is a scary body of work as processNextStarts() is not just in the base steps MapStep/FlatMapStep/BranchStep/etc., but littered throughout where extensions to the base steps are not easily done (e.g. barriers, etc.). Next, benchmark --- for every next there is an interrupt check (eek!).
The idea of being able to interrupt a Traversal is nice, but the implementation and performance details are probably going to prove daunting. Marko. http://markorodriguez.com On Oct 16, 2015, at 5:31 AM, Daniel Kuppitz <m...@gremlin.guru> wrote: > This is a nice one. Once we have that in place, we can probably/hopefully > also provide a hotkey to gracefully cancel queries in the REPL (w/o having > to exit the console itself). > > Cheers, > Daniel > > > On Fri, Oct 16, 2015 at 1:05 PM, Stephen Mallette <spmalle...@gmail.com> > wrote: > >> I'm going to frame this in the context of Gremlin Server but I think this >> issue generally applies to any threaded application development done with >> TinkerPop. >> >> Gremlin Server does a number of things to protect itself from crazy scripts >> (i.e. long run scripts that may have been accidentally or maliciously sent >> to it). The one thing it doesn't do perfectly is properly kill running >> scripts in the midst of a long run traversal. It attempts to stop a >> running script by interrupting the thread that is processing it, but if the >> thread is at a point in the script that doesn't respect Thread.interrupt(), >> it basically just chews up that thread until it reaches a spot that does. >> >> I think Traversal could do a better job respecting interrupts. Put in code >> speak, we should look to get this test to pass for all steps: >> >> @Test >> public void shouldRespectThreadInterruption() throws Exception { >> final AtomicBoolean exceptionThrown = new AtomicBoolean(false); >> final CountDownLatch startedIterating = new CountDownLatch(1000); >> final List<Integer> l = IntStream.range(0, >> 1000000).boxed().collect(Collectors.toList()); >> >> final Thread t = new Thread(() -> { >> try { >> __.inject(l).unfold().sideEffect(i -> >> startedIterating.countDown()).iterate(); >> fail("Should have respected the thread interrupt"); >> } catch (Exception ie) { >> exceptionThrown.set(ie instanceof RuntimeException); >> } >> }); >> >> t.start(); >> >> startedIterating.await(); >> t.interrupt(); >> >> t.join(); >> assertThat(exceptionThrown.get(), CoreMatchers.is(true)); >> } >> >> Two things to note...First, the changes to make this test pass shouldn't be >> "big". I got this test to pass with the addition of this line of code in >> FlatMapStep.processNextStart() on the first line after the start of the >> while(true). >> >> if(Thread.interrupted) throw new RuntimeException(); >> >> That single line inserted in the right places should get interrupt working >> properly across Traversal. >> >> Second, note that I'm throwing/asserting RuntimeException. I probably >> should be throwing a InterruptedException but that is a checked exception >> and that would really pollute up our Traversal code. So, my suggestion is >> to create our own "InterruptedRuntimeException" that we can trap separately >> that would potentially wrap an actual InterruptedException. >> >> I'd like to tack this issue in to 3.1.0 if possible - please let me know >> your thoughts if you have any. >>
