I haven't "dug in" for a while so this could be crazy talk but, the ProfileStrategy has a similar concern: add functionality without hindering normal performance. When enabled, ProfileStrategy injects a ProfileStep after every normal Step. So when profiling is enabled it works, but when it's not, there is no performance hit.
It seems like it would be similarly possible to inject a InterruptionStep that simply checks for interruption and throws when appropriate. This way none of the existing steps have to worry about the interruption logic and there is no performance hit when it is not enabled. That said, I'm not sure this would provide the checkpoints to ensure that interruption is handled w/ the necessary frequency. Bob On Fri, Oct 16, 2015 at 11:14 AM, Stephen Mallette <spmalle...@gmail.com> wrote: > Marko, we actually experimented with that InterruptStrategy idea before GA > (not quite as advanced as Matt described, but we tried). we ended up > gutting it for various reasons. Perhaps we could revisit that. > > I'm sensitive to the performance issue and the complexity, but I think even > some respect for interruption is worthwhile. I was offering a blanket > solution in suggesting "every step" but a partial solution that affords > some support for interruption would be welcome as well. > > On Fri, Oct 16, 2015 at 11:11 AM, Marko Rodriguez <okramma...@gmail.com> > wrote: > > > Ah…. an InterruptStrategy! … I'm liking that. Matt, can you say a bit > more > > about such a design? If you can wrap an entire traversal in its own > > isolated thread and then be able to kill it when you want, that would be > > pretty sweet --- however, does that mess up threaded transactions and the > > one-thead-per-transaction-model that TP3 current supports? > > > > Marko. > > > > http://markorodriguez.com > > > > On Oct 16, 2015, at 9:08 AM, Matt Frantz <matthew.h.fra...@gmail.com> > > wrote: > > > > > Perhaps a strategy could insert interrupt checks in appropriate places, > > > avoiding inner loops, or perhaps refactoring them into two-level loops > > > where we check for interrupt on the outer level. > > > > > > On Fri, Oct 16, 2015 at 6:58 AM, Marko Rodriguez <okramma...@gmail.com > > > > > wrote: > > > > > >> 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. > > >>>> > > >> > > >> > > > > >
