Test no thread leaks in engine and connectors #21470

findepi · 2024-04-09T14:57:39Z

losipiuk · 2024-04-09T20:47:28Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveModule.java

@@ -175,4 +178,16 @@ public boolean translateHiveViews(HiveConfig hiveConfig)
    {
        return hiveConfig.isTranslateHiveViews();
    }
+
+    public record ShutdownExecutors(


can we have wrapper around Executor, ScheduledExecutor, ExecutorService, ScheduledExecutorService ...
which delegates all the methods and puts @PreDestroy on shutdown?

Would that be nicer?

Something like #20960?

that's probably why i couldn't find the Cleanup class (got removed 0d4f855#r1514739127)

losipiuk

LGTM % suggestion on different way to close executors

hashhar

Cc: @kokosing you recently merged something around this area, might be useful to reuse.

martint

I have concerns about the new "assertNoThreadLeaked" infrastructure.

testing/trino-testing/src/main/java/io/trino/testing/ThreadAssertions.java

martint · 2024-04-09T22:07:41Z

testing/trino-testing/src/main/java/io/trino/testing/ThreadAssertions.java

+                try (T resource = resourceCreator.get()) {
+                    exerciseResource.accept(resource);
+                }
+                Thread[] threads = new Thread[clamp(threadGroup.activeCount(), 10, 1000)];


Why 10? 1000?

This is brittle and will result in false positives, false negatives, and potentially flaky tests. How does it account for all the JVM platform threads or framework threads that are not under Trino's control?

Also, relying on an unspecified naming convention ("ForkJoinPool.commonPool-worker-xxx") is not a good idea, as there's no guarantee it won't change arbitrarily.

You can do this to get the threads:

List<Thread> list = Thread.getAllStackTraces().keySet().stream() .filter(thread -> threadGroup.parentOf(thread.getThreadGroup())) .toList();

It is slower because the JVM gets the stack traces also, but it is more accurate.

Why 10? 1000?

10 -- at least no-zero, so that the next call is authoritative on live threads.
1000 -- arbitrarily chosen cap so that we don't print too many threads, if too many threads stay alive

This is brittle and will result in false positives, false negatives, and potentially flaky tests. How does it account for all the JVM platform threads or framework threads that are not under Trino's control?

This may be brittle and we may need to improve or remove the test.

false positives (successes) are acceptable from this test perspective.
false negatives (failures) are not acceptable and we may need to improve or remove the test.
potentially flaky tests -- thanks for pointing this out. You know how much i care about non-flaky tests and now i don't feel alone
account for JVM platform threads -- by inspecting only one ThraedGroup, created privately within the test.
framework threads -- by performing a warmup first (might not be sufficient if framework expires the threads, then we may eg need to add excludes for known non-leaks -- i had it initially)

You can do this to get the threads:

thanks @dain for the suggestion

It's generic, because i wanted to test cover both query runner and PlanTester.
If i introduce the public methods that expect these types, would it help?
If not, what would you do?

That would be better. However, we should look into getting rid of PlanTester. There's nothing it's doing that shouldn't be doable with StandaloneQueryRunner. Maybe the only thing is being able to specify which optimizers are instantiated, but I would argue that that's an improper way of testing optimizations.

Do you want me to remove PlanTester within this PR? Not sure I know exactly what this entails.

No, that would be a separate change. But that wouldn’t be a pre requisite for testing query runners alone.

OK, so i kept the test with PlanTester and made it impossible to provide anything other than PlanTester or QueryRunner

findepi · 2024-04-10T21:59:44Z

AC, PTAL

#21470 (comment) addressed

martint · 2024-04-10T23:27:23Z

testing/trino-testing/src/main/java/io/trino/testing/ThreadAssertions.java

+                String stackTraces = Arrays.stream(threads, 0, count)
+                        .filter(thread -> thread != Thread.currentThread())
+                        // Common ForkJoinPool threads are statically managed, not considered a leak
+                        .filter(thread -> !thread.getName().startsWith("ForkJoinPool.commonPool-worker-"))


Why would fork join pool thread show up in the list of threads for the thread group?

good question. Maybe the common FJP has a thread factory that doesn't set TG explicitly. i suspect it might be that new FJP threads inherit ThreadGroup from the thread interacting with the FJP.

martint · 2024-04-10T23:31:09Z

testing/trino-testing/src/main/java/io/trino/testing/ThreadAssertions.java

+                        // Common ForkJoinPool threads are statically managed, not considered a leak
+                        .filter(thread -> !thread.getName().startsWith("ForkJoinPool.commonPool-worker-"))
+                        // OkHttp TaskRunner is statically managed (okhttp3.internal.concurrent.TaskRunner), not considered a leak
+                        .filter(thread -> !thread.getName().equals("OkHttp TaskRunner"))


What does "statically managed" mean in this context? If the okhttp thread pools are not being closed, then how is that not a leak?

This is a pool living on a static field in the library, started lazily by the library. It would be a problem if we wanted to unload the classloader, but we already know classloader unloading won't really work (due to various libraries employing this pattern), and that's why duplicating plugin classloader was removed.

findepi · 2024-04-11T18:11:28Z

test failures are not related

findepi · 2024-04-13T05:53:14Z

AC. rebased to re-run, previous build failed due to flaky tests

findepi · 2024-04-16T10:54:25Z

Stress test green % "Upload test results" failing in every job.

findepi · 2024-04-16T12:25:04Z

Stress test green % workflow verification

findepi · 2024-04-16T14:56:30Z

another stress test round

Flaky tests due to insufficient synchronization in FileHiveMetastore #21121
and two legit failures related to this PR: test (plugin/trino-hudi), test (plugin/trino-iceberg)

findepi · 2024-04-16T19:23:56Z

running TestIcebergQueryRunner locally 1000 times i was able to get two failures, different than on the IC -- "task-driver-timeout-1" leaked & "worker-dynamic-catalog-manager-2" leaked. The first is a ScheduledExecutorService closed here:

trino/core/trino-main/src/main/java/io/trino/execution/SqlTaskManager.java

Line 341 in e5050c2

driverTimeoutExecutor.shutdownNow();

Here too, like in many other places, we do shutdownNow() without awaiting termination.
Second one is a similar case.

Fixing such leaks would be a bigger effort.

findepi · 2024-05-10T09:08:29Z

Continued at #21913

cla-bot bot added the cla-signed label Apr 9, 2024

github-actions bot added tests:hive hudi Hudi connector iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector bigquery BigQuery connector mongodb MongoDB connector labels Apr 9, 2024

findepi force-pushed the findepi/thread-leaks-13 branch 3 times, most recently from 76829cf to 8469dcd Compare April 9, 2024 15:31

findepi changed the title ~~Test for thread leaks~~ Fix thread leaks in Hive and Delta connectors Apr 9, 2024

findepi removed bigquery BigQuery connector mongodb MongoDB connector labels Apr 9, 2024

findepi force-pushed the findepi/thread-leaks-13 branch 2 times, most recently from 6ff24cd to 318600b Compare April 9, 2024 16:16

findepi changed the title ~~Fix thread leaks in Hive and Delta connectors~~ Fix thread leaks in Hive, Iceberg, Delta and Hudi connectors Apr 9, 2024

findepi force-pushed the findepi/thread-leaks-13 branch 2 times, most recently from 3a715e7 to c81c83a Compare April 9, 2024 18:44

losipiuk reviewed Apr 9, 2024

View reviewed changes

losipiuk approved these changes Apr 9, 2024

View reviewed changes

hashhar reviewed Apr 9, 2024

View reviewed changes

findepi force-pushed the findepi/thread-leaks-13 branch 2 times, most recently from 7411aca to 0bc8341 Compare April 9, 2024 21:43

martint previously requested changes Apr 9, 2024

View reviewed changes

findepi requested review from martint and dain April 10, 2024 06:53

This was referenced Apr 10, 2024

Closing binder cleanup #21477

Merged

Fix thread leaks in Hive, Iceberg, Delta and Hudi connectors #21490

Merged

findepi force-pushed the findepi/thread-leaks-13 branch from 0bc8341 to 07946b0 Compare April 10, 2024 12:33

findepi changed the title ~~Fix thread leaks in Hive, Iceberg, Delta and Hudi connectors~~ Fix thread leaks in Trino Apr 10, 2024

findepi mentioned this pull request Apr 10, 2024

Terminate WorkerDynamicCatalogManager's executor #21491

Merged

findepi force-pushed the findepi/thread-leaks-13 branch 3 times, most recently from 8c3dc8d to d480a6d Compare April 10, 2024 21:58

findepi changed the title ~~Fix thread leaks in Trino~~ Test no thread leaks in engine and connectors Apr 10, 2024

findepi added the no-release-notes This pull request does not require release notes entry label Apr 10, 2024

martint reviewed Apr 10, 2024

View reviewed changes

findepi force-pushed the findepi/thread-leaks-13 branch from d480a6d to 16fd619 Compare April 13, 2024 05:52

findepi force-pushed the findepi/thread-leaks-13 branch from 94aac1a to b9b68cd Compare April 16, 2024 19:38

findepi marked this pull request as draft April 16, 2024 19:39

findepi removed the tests:hive label Apr 18, 2024

findepi force-pushed the findepi/thread-leaks-13 branch from b9b68cd to cea89b3 Compare April 23, 2024 09:29

findepi force-pushed the findepi/thread-leaks-13 branch from cea89b3 to b813d23 Compare May 9, 2024 08:57

Test no thread leaks in engine and connectors

2477e6b

findepi force-pushed the findepi/thread-leaks-13 branch from b813d23 to 2477e6b Compare May 9, 2024 09:04

findepi mentioned this pull request May 10, 2024

Glue v2 #20657

Merged

findepi closed this May 10, 2024

findepi deleted the findepi/thread-leaks-13 branch May 10, 2024 09:07

findepi mentioned this pull request May 10, 2024

Test no thread leaks in engine and connectors #21913

Draft

findepi mentioned this pull request May 10, 2024

Fix thread leak in v1 GlueHiveMetastore #21912

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test no thread leaks in engine and connectors #21470

Test no thread leaks in engine and connectors #21470

findepi commented Apr 9, 2024 •

edited

Loading

losipiuk Apr 9, 2024

hashhar Apr 9, 2024

findepi Apr 9, 2024

findepi Apr 9, 2024

losipiuk left a comment

hashhar left a comment

martint left a comment

martint Apr 9, 2024

martint Apr 9, 2024

dain Apr 10, 2024

findepi Apr 10, 2024

findepi Apr 10, 2024

findepi Apr 10, 2024

martint Apr 10, 2024

findepi Apr 10, 2024

martint Apr 10, 2024

findepi Apr 10, 2024

findepi commented Apr 10, 2024

martint Apr 10, 2024

findepi Apr 11, 2024

martint Apr 10, 2024

findepi Apr 11, 2024

findepi commented Apr 11, 2024

findepi commented Apr 13, 2024

findepi commented Apr 16, 2024

findepi commented Apr 16, 2024

findepi commented Apr 16, 2024 •

edited

Loading

findepi commented Apr 16, 2024 •

edited

Loading

findepi commented May 10, 2024

Test no thread leaks in engine and connectors #21470

Test no thread leaks in engine and connectors #21470

Conversation

findepi commented Apr 9, 2024 • edited Loading

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

losipiuk left a comment

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

martint 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Apr 10, 2024

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

findepi commented Apr 11, 2024

findepi commented Apr 13, 2024

findepi commented Apr 16, 2024

findepi commented Apr 16, 2024

findepi commented Apr 16, 2024 • edited Loading

findepi commented Apr 16, 2024 • edited Loading

findepi commented May 10, 2024

findepi commented Apr 9, 2024 •

edited

Loading

findepi commented Apr 16, 2024 •

edited

Loading

findepi commented Apr 16, 2024 •

edited

Loading