Yeah, looks like benchmark-wise it's a wash, which is good. I wasn't aware of the difference between the static interrupted() and non-static isInterrupted(). I was wondering if in this case it should be isInterrupted(), but I think how you did it is good because it'll be evaluated within the traversal thread regardless.
--Ted On Mon, Apr 18, 2016 at 6:11 AM, Stephen Mallette <spmalle...@gmail.com> wrote: > A while back, I brought up the issue of being able to interrupt traversals: > > > https://pony-poc.apache.org/thread.html/e6477fc9c58d37a5bdcb5938a0eaa285456ad15aa39e16446290e2ff@1444993523@%3Cdev.tinkerpop.apache.org%3E > https://issues.apache.org/jira/browse/TINKERPOP-946 > > As a quick refresher, making Traversal respect Thread.interrupted() is > important as you otherwise can quite easily lock up applications like > Gremlin Server with a few poorly conceived or errant queries. We'd left > that last thread with liking the idea, but there were concerns about the > complexity of the changes and performance hits. > > Given that we now have gremlin-benchmark, I decided to see what the > performance hit would be for making this change. I took a rough stab at it > introducing Thread.interrupted() in all steps where it seemed to make sense > to do so and then ran the benchmark before and after the change. > > https://gist.github.com/spmallette/ed21267f2e7e17bb3fbd5a8d1a568d2b > > I'm not seeing a whole lot of difference between supporting this feature > and not supporting this feature. Here's the branch I implemented this in > in case you want to look around: > > https://github.com/apache/incubator-tinkerpop/tree/TINKERPOP-946 > > I'm not sure that my changes are completely bulletproof at this point, but > I'm reasonably sure that these changes would handle a good majority of > calls for thread interruption. I expect to re-target my branch at tp31 > (currently from master so that i could use the benchmark suite) if this > becomes a pull request. > > Any thoughts on the benchmark, the implementation, etc? >
