You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by Stephen Mallette <sp...@gmail.com> on 2015/10/16 13:05:23 UTC
[DISCUSS] Traversal respecting Thread.interrupt()
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.
Re: [DISCUSS] Traversal respecting Thread.interrupt()
Posted by Stephen Mallette <sp...@gmail.com>.
> One design along the lines of what Bob just said would to interleave InterruptionSteps
everywhere.
That was the approach Marko and I had pre-GA but didn't like as it was too
easy to write some Gremlin that wouldn't interrupt.
> It is still possible that we want to propagate interruptions to code that executes
within a single step
That's what I'd like to see this extend to somehow because you aren't
always aware that you've run afoul of something until its in execution mode.
> for that, we would need a provider API that they could call to check for interrupt
requests.
I'm not sure I see why we need a provider API. If providers want to check
for interruption in their implementations wouldn't they just check:
if (Thread.interrupted())
// stop
> 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.
could you talk more here about what you had in mind?
On Fri, Oct 16, 2015 at 1:00 PM, Matt Frantz <ma...@gmail.com>
wrote:
> 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 <bo...@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 <spmallette@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 <
> okrammarko@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.frantz@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 <
> > okrammarko@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 <me...@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 <
> > > > spmallette@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.
> > > > >>>>
> > > > >>
> > > > >>
> > > >
> > > >
> > >
> >
>
Re: [DISCUSS] Traversal respecting Thread.interrupt()
Posted by Matt Frantz <ma...@gmail.com>.
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 <bo...@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 <sp...@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 <ok...@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 <ma...@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 <
> okrammarko@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 <me...@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 <
> > > spmallette@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.
> > > >>>>
> > > >>
> > > >>
> > >
> > >
> >
>
Re: [DISCUSS] Traversal respecting Thread.interrupt()
Posted by Bob Briody <bo...@datastax.com>.
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 <sp...@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 <ok...@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 <ma...@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 <okrammarko@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 <me...@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 <
> > spmallette@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.
> > >>>>
> > >>
> > >>
> >
> >
>
Re: [DISCUSS] Traversal respecting Thread.interrupt()
Posted by Stephen Mallette <sp...@gmail.com>.
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 <ok...@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 <ma...@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 <ok...@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 <me...@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 <
> spmallette@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.
> >>>>
> >>
> >>
>
>
Re: [DISCUSS] Traversal respecting Thread.interrupt()
Posted by Marko Rodriguez <ok...@gmail.com>.
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 <ma...@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 <ok...@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 <me...@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 <sp...@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.
>>>>
>>
>>
Re: [DISCUSS] Traversal respecting Thread.interrupt()
Posted by Matt Frantz <ma...@gmail.com>.
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 <ok...@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 <me...@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 <sp...@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.
> >>
>
>
Re: [DISCUSS] Traversal respecting Thread.interrupt()
Posted by Marko Rodriguez <ok...@gmail.com>.
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 <me...@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 <sp...@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.
>>
Re: [DISCUSS] Traversal respecting Thread.interrupt()
Posted by Daniel Kuppitz <me...@gremlin.guru>.
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 <sp...@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.
>