Add thread timeout that interrupts query analyzer thread #17650

vanekjar · 2022-04-14T22:33:27Z

It was discovered that runaway regex can run for very long or even end up in infinite loop and block the query analyzer thread. The fix introduces a thread timeout that interrupts the runaway regex matcher after a certain timeout.

Example of a query that can cause issues:

select REGEXP_EXTRACT('runaway_regex-is-evaluated-infinitely - xxx"}', '.*runaway_(.*?)+-+xxx.*')

Test plan - unit test

== RELEASE NOTES ==

General Changes
* Add support to set timeout on the query analyzer runtime to prevent long running query analysis like complex regular expressions. The timeout duration can be set by the configuration property `planner.query-analyzer-timeout` or session property `query_analyzer_timeout`.

linux-foundation-easycla · 2022-04-14T22:33:30Z

The committers listed above are authorized under a signed CLA.

✅ login: vanekjar / name: Jaromir Vanek (66d0456dab2bdd7bcb179ce263fd76a1f3d44cc2)

aweisberg

This is great!

The release note should mention the actual configuration property, also want it in the docs. I think the layout there doesn't present a good location for this. Maybe add a Planner section for this? I think the name might also need some rethinking, but I am not 100% sure what it should be.

For the PR description. It's not just REGEXP_EXTRACT it's generally possible to write a regular expression and use several functions. I would just say regular expressions can end up being very long running or infinite.

aweisberg · 2022-04-15T00:01:33Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java

@@ -224,6 +224,8 @@

    private double hyperloglogStandardErrorWarningThreshold = 0.004;

+    private Duration queryAnalyzerTimeout = new Duration(30, SECONDS);


The query analyzer is basically the whole planner and there is a 3 minute timeout on the iterative optimizer. I would say this should default to the same as the iterative optimizer timeout otherwise the cases where the iterative optimizer takes a long time will also get canceled/interrupt.

aweisberg · 2022-04-15T00:02:20Z

presto-main/src/main/java/com/facebook/presto/execution/ThreadWatchdog.java

+     */
+    public ThreadWatchdog(Thread thread, ScheduledExecutorService executor, Duration timeout)
+    {
+        this.thread = requireNonNull(thread);


We always provide a message saying which parameter was null.

aweisberg · 2022-04-15T00:03:11Z

presto-main/src/main/java/com/facebook/presto/execution/ThreadWatchdog.java

+    private void start()
+    {
+        this.future = executor.schedule(
+                thread::interrupt, timeout.toMillis(), TimeUnit.MILLISECONDS);


In j.u.c usage you normally always do nanoseconds to allow the highest precision timings to flow through. It doesn't matter here though.

aweisberg · 2022-04-15T00:04:01Z

presto-main/src/main/java/com/facebook/presto/execution/SqlQueryExecution.java

@@ -356,7 +360,10 @@ public int getRunningTaskCount()
    @Override
    public void start()
    {
-        try (SetThreadName ignored = new SetThreadName("Query-%s", stateMachine.getQueryId())) {
+        try (SetThreadName ignored = new SetThreadName("Query-%s", stateMachine.getQueryId());
+                ThreadWatchdog w = new ThreadWatchdog(Thread.currentThread(),


I think it's less risky to just scope this to the query analyzer and not all the other methods including starting the scheduler.

That said there might be value in being able to interrupt the entire thing. @highker WDYT?

I assumed the thread may get stuck anywhere, not just the specific discovered case. But considering I named the property query_analyzer_timeout, it probably makes sense to wrap only the analyzer part.

aweisberg · 2022-04-15T00:11:15Z

presto-main/src/main/java/com/facebook/presto/execution/SqlQueryExecution.java

+        try (SetThreadName ignored = new SetThreadName("Query-%s", stateMachine.getQueryId());
+                ThreadWatchdog w = new ThreadWatchdog(Thread.currentThread(),
+                        watchdogExecutor,
+                        SystemSessionProperties.getQueryAnalyzerTimeout(getSession()))) {


We static import these SystemSessionProperties methods

aweisberg · 2022-04-15T00:11:26Z

presto-main/src/main/java/com/facebook/presto/execution/ThreadWatchdog.java

+{
+    private final Thread thread;
+    private final ScheduledExecutorService executor;
+    private final Duration timeout;


This class doesn't need any of these since starting is implicit in construction. I would just remove the start method and inline creating the task.

aweisberg · 2022-04-15T00:12:32Z

presto-main/src/main/java/com/facebook/presto/server/CoordinatorModule.java

+    public static ScheduledExecutorService createWatchdogExecutor()
+    {
+        ScheduledThreadPoolExecutor executor =
+                new ScheduledThreadPoolExecutor(1, threadsNamed("thread-watchdog"));


Use daemonThreadsNamed

vanekjar · 2022-04-15T01:15:36Z

Thanks for the quick review. I fixed all the comments in the code and updated the doc. Please let me know if I matched the wording correctly.

highker

Can we squash the commits into one? Note that we do encourage have multiple small commits. But each commit should be logically self-contained. For this change, one commit is good enough.
Can we wrap the commit title and message up to 72 characters per line? Please follow the commit guideline here: https://cbea.ms/git-commit/

highker · 2022-04-15T05:45:03Z

presto-main/src/main/java/com/facebook/presto/execution/ThreadWatchdog.java

+ * ThreadWatchdog spins up a background task
+ * that interrupts given thread after the timeout expires.
+ */
+public class ThreadWatchdog


Let's call it TimeoutThread. Same for all other places in this PR. Watchdog doesn't sound very formal.

highker · 2022-04-15T05:47:47Z

presto-main/src/main/java/com/facebook/presto/execution/ThreadWatchdog.java

+        this.future = executor.schedule(
+                thread::interrupt, timeout.roundTo(NANOSECONDS), NANOSECONDS);


put into the same line

highker · 2022-04-15T05:48:15Z

presto-main/src/main/java/com/facebook/presto/execution/ForWatchdog.java

+@Retention(RUNTIME)
+@Target({FIELD, PARAMETER, METHOD})
+@Qualifier
+public @interface ForWatchdog


ForTimeoutThread; same for everywhere else

highker · 2022-04-15T05:50:45Z

presto-main/src/main/java/com/facebook/presto/execution/SqlQueryExecution.java

+                PlanRoot plan;
+
+                // setup a thread watchdog in case query analyzer ends up in an infinite loop
+                try (ThreadWatchdog w = new ThreadWatchdog(Thread.currentThread(),


move Thread.currentThread() to its own line. We leave the first line empty for function parameters if too long.

instead of calling it w, we could call it unused or thread. Usually we spell the full name for variables and definitions

highker · 2022-04-15T05:51:38Z

presto-main/src/main/java/com/facebook/presto/server/CoordinatorModule.java

+        ScheduledThreadPoolExecutor executor =
+                new ScheduledThreadPoolExecutor(1, daemonThreadsNamed("thread-watchdog"));


put into the same line

if we only have one thread, would that cause starvation if some bad thread blocks the legit ones?

if we only have one thread, would that cause starvation if some bad thread blocks the legit ones?

TimeoutThread only executes Thread#interrupt and finishes. It shouldn't get blocked. I believe it's safer than spawning large amount of threads by mistake.

highker · 2022-04-15T05:51:53Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java

+    @ConfigDescription("Maximum running time for the query analyzer in case" +
+            "the processing takes too long or is stuck in an infinite loop.")


put into same line

highker · 2022-04-15T05:53:27Z

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestDistributedQueries.java

+    @Test(expectedExceptions = RuntimeException.class, expectedExceptionsMessageRegExp = "Regexp matching interrupted",
+            timeOut = 30_000)


put into the same line; or if it's too long, keep the first line empty and then one param per line

vanekjar · 2022-04-19T17:52:44Z

@highker Thank you for your thorough review and guidance. I updated the PR accordingly.

highker

Thanks for the contribution. The test failure is related. Please take a look

highker · 2022-04-19T21:19:58Z

presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestFeaturesConfig.java

@@ -194,7 +194,8 @@ public void testDefaults()
                .setMaxStageCountForEagerScheduling(25)
                .setHyperloglogStandardErrorWarningThreshold(0.004)
                .setPreferMergeJoin(false)
-                .setRoundRobinShuffleBeforePartialDistinctLimit(false));
+                .setRoundRobinShuffleBeforePartialDistinctLimit(false)
+                .setQueryAnalyzerTimeout(new Duration(30, SECONDS)));


This will fail the test

Thanks for the heads up. Just fixed.

It was discovered that runaway regex can run for very long or even end up in infinite loop and block the query analyzer thread. The fix introduces a thread timeout that interrupts the runaway regex matcher after a certain timeout.

vanekjar force-pushed the thread-watchdog branch 2 times, most recently from 1d32620 to 908b40d Compare April 14, 2022 23:23

aweisberg self-requested a review April 14, 2022 23:56

vanekjar force-pushed the thread-watchdog branch from 908b40d to 1e53011 Compare April 15, 2022 00:04

aweisberg suggested changes Apr 15, 2022

View reviewed changes

aweisberg requested a review from highker April 15, 2022 00:20

vanekjar force-pushed the thread-watchdog branch from 45786d5 to baf5df1 Compare April 15, 2022 01:14

vanekjar force-pushed the thread-watchdog branch 2 times, most recently from 43d5ee2 to 7652399 Compare April 15, 2022 01:28

highker requested review from NikhilCollooru, pgupta2 and souravpal April 15, 2022 02:17

highker reviewed Apr 15, 2022

View reviewed changes

vanekjar changed the title ~~Add thread watchdog that interrupts query analyzer thread~~ Add thread timeout that interrupts query analyzer thread Apr 19, 2022

vanekjar force-pushed the thread-watchdog branch 3 times, most recently from 36bfe61 to 64794a3 Compare April 19, 2022 17:51

highker self-requested a review April 19, 2022 19:00

highker approved these changes Apr 19, 2022

View reviewed changes

highker self-assigned this Apr 19, 2022

vanekjar force-pushed the thread-watchdog branch from 64794a3 to 39ef82e Compare April 19, 2022 21:28

highker merged commit 1660a27 into prestodb:master Apr 19, 2022

vanekjar deleted the thread-watchdog branch April 19, 2022 22:47

mshang816 mentioned this pull request May 17, 2022

Add release notes for 0.273 #17775

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add thread timeout that interrupts query analyzer thread #17650

Add thread timeout that interrupts query analyzer thread #17650

vanekjar commented Apr 14, 2022 •

edited by highker

Loading

linux-foundation-easycla bot commented Apr 14, 2022 •

edited

Loading

aweisberg left a comment

aweisberg Apr 15, 2022

aweisberg Apr 15, 2022

aweisberg Apr 15, 2022

aweisberg Apr 15, 2022

vanekjar Apr 15, 2022

aweisberg Apr 15, 2022

aweisberg Apr 15, 2022

aweisberg Apr 15, 2022

vanekjar commented Apr 15, 2022 •

edited

Loading

highker left a comment

highker Apr 15, 2022

highker Apr 15, 2022

highker Apr 15, 2022

highker Apr 15, 2022

highker Apr 15, 2022

vanekjar Apr 19, 2022

highker Apr 15, 2022

highker Apr 15, 2022

vanekjar commented Apr 19, 2022

highker left a comment

highker Apr 19, 2022

vanekjar Apr 19, 2022

		@@ -224,6 +224,8 @@

		private double hyperloglogStandardErrorWarningThreshold = 0.004;

		private Duration queryAnalyzerTimeout = new Duration(30, SECONDS);

		this.future = executor.schedule(
		thread::interrupt, timeout.roundTo(NANOSECONDS), NANOSECONDS);

		ScheduledThreadPoolExecutor executor =
		new ScheduledThreadPoolExecutor(1, daemonThreadsNamed("thread-watchdog"));

		@ConfigDescription("Maximum running time for the query analyzer in case" +
		"the processing takes too long or is stuck in an infinite loop.")

		@Test(expectedExceptions = RuntimeException.class, expectedExceptionsMessageRegExp = "Regexp matching interrupted",
		timeOut = 30_000)

Add thread timeout that interrupts query analyzer thread #17650

Add thread timeout that interrupts query analyzer thread #17650

Conversation

vanekjar commented Apr 14, 2022 • edited by highker Loading

linux-foundation-easycla bot commented Apr 14, 2022 • edited Loading

aweisberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanekjar commented Apr 15, 2022 • edited Loading

highker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanekjar commented Apr 19, 2022

highker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanekjar commented Apr 14, 2022 •

edited by highker

Loading

linux-foundation-easycla bot commented Apr 14, 2022 •

edited

Loading

vanekjar commented Apr 15, 2022 •

edited

Loading