Adds Compile-Time `Thread.interrupted()` checks #398

dlurton · 2021-04-30T23:05:36Z

A few times over the past few years we've received reports from a PartiQL service where with a customer sending a very large query (in terms of size of the SQL), where PartiQL would take a long time (>30s) to parse, rewrite, validate, and compile.

Although we have optimized every case where this has happened, this PR applies a mitigation measure which can be used in case it happens again. I have identified several "hot" loops where the state of Thread.interrupted() can be polled and if set an InterruptedException is thrown. This is the hope that if such a query is encountered again, it can be aborted by the calling code rather than risk causing service outages. This approach still might miss loops that we did not anticipate would become hot in the case of unexpected user input, but at least this should allow most such scenarios to be aborted. If it later discovered that we missed such a hot loop in the future, it is a simple matter to add a call to checkThreadInterrupted().

What this PR does not do is add an evaluation-time poll for Thread.interrupted(). That will come in another PR if it is later determined to be needed.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

almann

LGTM, some minor questions below.

lang/src/org/partiql/lang/ast/AstDeserialization.kt

lang/src/org/partiql/lang/ast/passes/AstRewriterBase.kt

lang/src/org/partiql/lang/ast/passes/AstVisitor.kt

lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt

lang/src/org/partiql/lang/eval/visitors/VisitorTransformBase.kt

alancai98

LGTM overall. Only have a couple nits/suggestions. Also, should these checks also be added to the v0.1.5 tag @dlurton ?

lang/src/org/partiql/lang/CompilerPipeline.kt

lang/src/org/partiql/lang/ast/passes/AstRewriterBase.kt

abhikuhikar

Some minor nits.
Can you also add an interrupt check in the function validateTopLevelNodes in SqlParser?
Since it recurses over the entire tree, I remember it had very "high flames" in the CPU profiler.

lang/src/org/partiql/lang/CompilerPipeline.kt

lang/src/org/partiql/lang/ast/AstDeserialization.kt

lang/src/org/partiql/lang/ast/ast.kt

dlurton · 2021-05-04T23:59:29Z

Can you also add an interrupt check in the function validateTopLevelNodes in SqlParser?

-- Added a check to validateTopLevelNodes

dlurton · 2021-05-05T00:00:13Z

LGTM overall. Only have a couple nits/suggestions. Also, should these checks also be added to the v0.1.5 tag @dlurton ?

That's next after this has been merged to master.

codecov-commenter · 2021-05-05T00:15:05Z

Codecov Report

Merging #398 (3bc9116) into master (eee99ae) will increase coverage by 0.02%.
The diff coverage is 94.11%.

@@             Coverage Diff              @@
##             master     #398      +/-   ##
============================================
+ Coverage     81.88%   81.91%   +0.02%     
- Complexity     1350     1351       +1     
============================================
  Files           168      169       +1     
  Lines         10491    10508      +17     
  Branches       1737     1738       +1     
============================================
+ Hits           8591     8608      +17     
  Misses         1360     1360              
  Partials        540      540

Flag	Coverage Δ	Complexity Δ
CLI	`18.18% <ø> (ø)`	`21.00 <ø> (ø)`
EXAMPLES	`74.85% <ø> (ø)`	`29.00 <ø> (ø)`
LANG	`84.41% <94.11%> (+0.02%)`	`1144.00 <70.00> (+1.00)`
PTS	`∅ <ø> (∅)`	`0.00 <ø> (ø)`
TEST_SCRIPT	`79.68% <ø> (ø)`	`157.00 <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
lang/src/org/partiql/lang/ast/passes/AstVisitor.kt	`0.00% <ø> (ø)`	`0.00 <0.00> (ø)`
.../src/org/partiql/lang/util/ThreadInterruptUtils.kt	`50.00% <50.00%> (ø)`	`0.00 <0.00> (?)`
lang/src/org/partiql/lang/CompilerPipeline.kt	`88.37% <100.00%> (+5.03%)`	`0.00 <0.00> (ø)`
...ang/src/org/partiql/lang/ast/AstDeserialization.kt	`86.09% <100.00%> (+0.04%)`	`0.00 <0.00> (ø)`
lang/src/org/partiql/lang/ast/AstSerialization.kt	`85.98% <100.00%> (+0.03%)`	`0.00 <0.00> (ø)`
...ng/src/org/partiql/lang/ast/ExprNodeToStatement.kt	`93.06% <100.00%> (+0.02%)`	`0.00 <0.00> (ø)`
...ng/src/org/partiql/lang/ast/StatementToExprNode.kt	`96.67% <100.00%> (+0.01%)`	`0.00 <0.00> (ø)`
lang/src/org/partiql/lang/ast/ast.kt	`89.08% <100.00%> (ø)`	`0.00 <0.00> (ø)`
...src/org/partiql/lang/ast/passes/AstRewriterBase.kt	`100.00% <100.00%> (ø)`	`124.00 <21.00> (ø)`
lang/src/org/partiql/lang/ast/passes/AstWalker.kt	`71.14% <100.00%> (+0.19%)`	`29.00 <0.00> (ø)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eee99ae...3bc9116. Read the comment docs.

abhikuhikar

LGTM. Thanks.

almann

For the future, I recommend that you try to avoid rebasing/squashing commits after you get feedback as it makes it impossible for the reviewer to do inter-diffs or only look at what you changed.

alancai98

Noticed one small typo. Rest of changes LGTM

alancai98 · 2021-05-05T01:26:57Z

lang/src/org/partiql/lang/eval/visitors/VisitorTransformBase.kt

+ * to [transformExpr].
+ *
+ * All transforms should derive from this class instead of [PartiqlAst.VisitorTransform] so that they can
+ * be interrupted of they take a long time to process large ASTs.


typo: "be interrupted if..."

dlurton · 2021-05-05T15:53:06Z

For the future, I recommend that you try to avoid rebasing/squashing commits after you get feedback as it makes it impossible for the reviewer to do inter-diffs or only look at what you changed.

Yes, sorry, I realized that too late.

* Add Thread.interrupted() checks

…403) * Adds Compile-Time `Thread.interrupted()` checks (#398) * Add Thread.interrupted() checks * Reduce unit test memory usage * Remove `@Deprecated`

dlurton requested review from alancai98, almann and abhikuhikar and removed request for alancai98 and almann April 30, 2021 23:07

dlurton self-assigned this Apr 30, 2021

almann previously approved these changes May 4, 2021

View reviewed changes

alancai98 previously approved these changes May 4, 2021

View reviewed changes

lang/src/org/partiql/lang/CompilerPipeline.kt Outdated Show resolved Hide resolved

lang/src/org/partiql/lang/ast/passes/AstRewriterBase.kt Outdated Show resolved Hide resolved

abhikuhikar suggested changes May 4, 2021

View reviewed changes

dlurton mentioned this pull request May 4, 2021

Add Thread.interrupted() checks to generated Kotlin code #400

Closed

dlurton dismissed stale reviews from alancai98 and almann via 41d2e15 May 4, 2021 23:58

dlurton force-pushed the compile-time-interruption branch from c12f36d to 41d2e15 Compare May 4, 2021 23:58

Add Thread.interrupted() checks

3bc9116

dlurton force-pushed the compile-time-interruption branch from 41d2e15 to 3bc9116 Compare May 5, 2021 00:08

dlurton requested review from abhikuhikar, alancai98 and almann May 5, 2021 00:15

abhikuhikar previously approved these changes May 5, 2021

View reviewed changes

almann previously approved these changes May 5, 2021

View reviewed changes

alancai98 previously approved these changes May 5, 2021

View reviewed changes

Update VisitorTransformBase.kt

8c8b5e5

dlurton dismissed stale reviews from alancai98, almann, and abhikuhikar via 8c8b5e5 May 5, 2021 15:51

dlurton requested a review from alancai98 May 5, 2021 15:51

alancai98 approved these changes May 5, 2021

View reviewed changes

dlurton merged commit 7926b29 into master May 5, 2021

dlurton deleted the compile-time-interruption branch May 6, 2021 21:11

dlurton added a commit that referenced this pull request May 7, 2021

Adds Compile-Time Thread.interrupted() checks (#398)

a7b3855

* Add Thread.interrupted() checks

This was referenced May 7, 2021

Cherry-pick PR #398 to v0.1.6 branch and optimize test memory usage #403

Merged

Reduce memory usage of Thread.interrupted() tests #404

Merged

dlurton added this to the v0.3.0 milestone Jun 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Compile-Time `Thread.interrupted()` checks #398

Adds Compile-Time `Thread.interrupted()` checks #398

dlurton commented Apr 30, 2021 •

edited

Loading

almann left a comment

alancai98 left a comment

abhikuhikar left a comment

dlurton commented May 4, 2021

dlurton commented May 5, 2021

codecov-commenter commented May 5, 2021 •

edited

Loading

abhikuhikar left a comment

almann left a comment

alancai98 left a comment

alancai98 May 5, 2021

dlurton May 5, 2021

dlurton commented May 5, 2021

Adds Compile-Time Thread.interrupted() checks #398

Adds Compile-Time Thread.interrupted() checks #398

Conversation

dlurton commented Apr 30, 2021 • edited Loading

almann left a comment

Choose a reason for hiding this comment

alancai98 left a comment

Choose a reason for hiding this comment

abhikuhikar left a comment

Choose a reason for hiding this comment

dlurton commented May 4, 2021

dlurton commented May 5, 2021

codecov-commenter commented May 5, 2021 • edited Loading

Codecov Report

abhikuhikar left a comment

Choose a reason for hiding this comment

almann left a comment

Choose a reason for hiding this comment

alancai98 left a comment

Choose a reason for hiding this comment

alancai98 May 5, 2021

Choose a reason for hiding this comment

dlurton May 5, 2021

Choose a reason for hiding this comment

dlurton commented May 5, 2021

Adds Compile-Time `Thread.interrupted()` checks #398

Adds Compile-Time `Thread.interrupted()` checks #398

dlurton commented Apr 30, 2021 •

edited

Loading

codecov-commenter commented May 5, 2021 •

edited

Loading