One design along the lines of what Bob just said would to interleave InterruptionSteps everywhere. We could also have a "NoInterrupt" step which would allow the traversal author to specify "critical sections" that should not be interrupted, potentially for performance optimization.
It is still possible that we want to propagate interruptions to code that executes within a single step, especially ones that access the graph. For that, we would need a provider API that they could call to check for interrupt requests. This would be opt-in for providers; failing to check for interruptions essentially limits the worst-case response time to the interrupt request. As to the threading model, I believe the use case is for interrupt requests originating in some side channel, e.g. an admin interface. It would require that some synchronization mechanism be used in the interrupt flag. There are ways to rate-limit such checks from the query execution thread so that we aren't continuously synchronizing to check for an interruption. To deal with the single-threaded case, you could have an API in which every call were effectively time-boxed, requiring the caller to issue "keep going" calls. This is a more natural architecture for an event loop type of virtual machine. On Fri, Oct 16, 2015 at 9:16 AM, Bob Briody <bob.bri...@datastax.com> wrote: > 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. > > > >>>> > > > >> > > > >> > > > > > > > > >
