Introduce ClosingBinder #20960

kokosing · 2024-03-06T15:43:47Z

Introduce ClosingBinder

It is easy to make a mistake when closing a resource on a
method in the guice module annotated with @PreDestroy. Such method won't
be called and resource may leak. This does not affect
production, but it may leak resources during the test.

ClosingBinder is created in order to help mitigate this problem by introducing
simple utility to close resources in guice modules.

kokosing · 2024-03-06T15:44:22Z

I haven't migrated all use cases or haven't fixed all wrong places yet. I wanted to first discuss the proposed solution.

dominikzalewski

Not sure if this is the 'discussion' you were expecting, but here it is.

dominikzalewski · 2024-03-06T15:50:31Z

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/ExecutorServiceCleanupBinder.java

+    public ExecutorServiceCleanupBinder(Binder binder)
+    {
+        multibinder = newSetBinder(binder, ExecutorService.class, ForCleanup.class);
+        binder.bind(Cleanup.class).asEagerSingleton();


I'm not a guice expert, but I'd check whether binding as eager singleton means that it will instantiate with the multibinder in the state that it is at this point in time, or whether it will instantiate once it iterated all the modules for any additional multibinding.

Since the cleanup class constructor depends on the set of executors, it will be created after this set (this is dependency in the graph)

dominikzalewski · 2024-03-06T15:51:28Z

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/ExecutorServiceCleanupBinder.java

+    @Retention(RUNTIME)
+    @Target({FIELD, PARAMETER, METHOD})
+    @BindingAnnotation
+    private @interface ForCleanup {}


NIT: maybe 'forCleanup' is to broad? 'forExecutorCleanup'?

It is fine to me, because it is private annotation. Also now I also used this for closeables too.

dominikzalewski · 2024-03-06T15:55:32Z

core/trino-main/src/main/java/io/trino/server/CoordinatorModule.java

@@ -363,7 +363,11 @@ List<OutputStatsEstimatorFactory> getCompositeOutputDataSizeEstimatorDelegateFac
        install(new QueryExecutionFactoryModule());

        // cleanup
-        binder.bind(ExecutorCleanup.class).asEagerSingleton();
+        executorServiceCleanupBinder(binder)


are we now going to centralize this registration? That is, if we have an executor that we want to cleanly shutdown in say troubleshooting module, we'd need to move the binding here?

if I misunderstood, then calling 'executorServiceCleanupBinder' would still work when called in many places? (in terms of eager singleton and such).

Will this be added in worker module as well?

The biggest problem we see is in test code. I'm missing a code sample or a text explanation that would show how does it improve things for tests?

Does this technique help with code outside of trino OSS (also plugins)?

troubleshooting module

What is "troubleshooting module"? I cannot find it in Trino.

are we now going to centralize this registration?

No.

then calling 'executorServiceCleanupBinder' would still work when called in many places?

Yes.

Will this be added in worker module as well?

If needed in can be added there too.

Does this technique help with code outside of trino OSS (also plugins)?

I think so.

The biggest problem we see is in test code. I'm missing a code sample or a text explanation that would show how does it improve things for tests?

Added test. Thanks.

dominikzalewski · 2024-03-06T15:57:37Z

core/trino-main/src/main/java/io/trino/server/CoordinatorModule.java

@@ -460,30 +464,4 @@ private void bindLowMemoryTaskKiller(LowMemoryTaskKillerPolicy policy, Class<? e
                        .to(clazz)
                        .in(Scopes.SINGLETON)));
    }
-
-    public static class ExecutorCleanup


can you explain how is your modification improving over what we had before here?

It was just a code extraction. No improvement here. But see the last commit now.

lukasz-stec · 2024-03-06T16:12:04Z

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/ExecutorServiceCleanupBinder.java

+import static java.lang.annotation.ElementType.PARAMETER;
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+
+public class ExecutorServiceCleanupBinder


how can we make sure this is used for all executors?

We cannot :(

Potentially, we can introduce a static rule that @PreDestroy annotation cannot be used directly in classes that ends with *Module.java. I am not sure if it is easy to be done with the modernizer.

I did take a look into modernizer and I didn't find anything that I can easily use :(

core/trino-main/src/main/java/io/trino/server/CoordinatorModule.java

kokosing · 2024-03-08T13:44:36Z

Comments addressed. Please take a look again. Thank you! :)

dominikzalewski

I like the abstraction you introduced. +1 for testing.
Left a limited number of 'consider for the future' comments, but generally can go without addressing them. Thank you for handling this nicely.

dominikzalewski · 2024-03-08T15:55:22Z

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/ClosingBinder.java

+        public void shutdown()
+                throws IOException
+        {
+            executors.forEach(ExecutorService::shutdownNow);


I'm not sure if the shutdownNow is the preferred behaviour. I can imagine that some folks might want to implement graceful shutdown with a timeout?

Do we try to call the 'shutdown' or 'close' methods on all the objects even if the first one in the loop throws an exception?

I think it is desired behavior. At this point when we stop the lifecycle all the processing should be done or abandoned. Also this code fallows how it was already implemented in places where there were no resource leaks. I prefer to change one thing at a time.

We could make this more generic and explicit by having a single registration type

public ClosingBinder register(Class<?> type, Callable callable) ... public ClosingBinder register(Class<?> type, Class<? extends Annotation> annotation, Callable callable) ...

Then the callers would provide the action

.register(ExecutorService.class, ForAsyncHttp.class, ExecutorService::shutdownNow)

I have tried to make it generic and I am unable to implement in a simple way. This generic methods requires a possibility to store a different closure method for a resource instance. To implement this I need to store some kind of mapping of a resource instance to its closure method.

The current implementation is simple as executor service will be closed only in a single way and closeable is also closed in single way.

lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/TestClosingBinder.java

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/ClosingBinder.java

electrum · 2024-03-08T20:33:07Z

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/ClosingBinder.java

+    {
+        executors = newSetBinder(binder, ExecutorService.class, ForCleanup.class);
+        closeables = newSetBinder(binder, Closeable.class, ForCleanup.class);
+        binder.bind(Cleanup.class).in(Scopes.SINGLETON);


This should be asEagerSingleton() since it should always be created

ping @kokosing . LGTM otherwise.

BTW I had a Guice question - according to table at https://github.com/google/guice/wiki/Scopes#eager-singletons seems all are equivalent - what do I miss? We seem to inherit PRODUCTION from https://github.com/airlift/airlift/blob/3f2f819f9bc08d848c45c3508e906f3fabdf8d3d/bootstrap/src/main/java/io/airlift/bootstrap/Bootstrap.java#L264

You are correct that in practice it won't make a difference, but bindings shouldn't make assumptions about the stage Guice is running in. It also makes the intent more clear.

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/ClosingBinder.java

kokosing · 2024-03-12T15:43:17Z

Comments addressed, please take a look.

kokosing · 2024-03-15T11:47:19Z

@electrum @hashhar Can you please take a look? My impression is that we can always change the API of the ClosingBinder, but let's fix the leaks sooner not later.

hashhar

LGTM.

As of today there's no cases where something other than shutdownNow is called anyway.

It is easy to make a mistake when closing a resource on a method in the guice module annotated with @PreDestroy. Such method won't be called and resource may leak. This does not affect production, but it may leak resources during the test. ClosingBinder is created in order to help mitigate this problem by introducing simple utility to close resources in guice modules.

There is no logic difference here. This commit replaces existing code with usage of closing binder without changing any logic.

kokosing · 2024-03-15T13:18:19Z

As of today there's no cases where something other than shutdownNow is called anyway.

There is schema registry client that was not closed.

hashhar · 2024-03-15T14:16:10Z

There is schema registry client that was not closed.

Yes, good catch.

I meant that for executors other than shutdownNow we don't need another close mechanism as of today so I'm fine with ignoring David's comment for now.

findepi · 2024-04-09T21:44:52Z

nice, thanks @kokosing

cla-bot bot added the cla-signed label Mar 6, 2024

kokosing requested a review from lukasz-stec March 6, 2024 15:43

kokosing marked this pull request as draft March 6, 2024 15:43

kokosing requested a review from dominikzalewski March 6, 2024 15:44

dominikzalewski reviewed Mar 6, 2024

View reviewed changes

lukasz-stec reviewed Mar 6, 2024

View reviewed changes

kokosing force-pushed the origin/master/004 branch from adb4dc8 to 292c1d0 Compare March 8, 2024 13:39

github-actions bot added tests:hive hive Hive connector labels Mar 8, 2024

kokosing changed the title ~~Introduce ExecutorServiceCleanupBinder~~ Introduce ClosingBinder Mar 8, 2024

kokosing requested a review from hashhar March 8, 2024 13:40

kokosing force-pushed the origin/master/004 branch from 292c1d0 to 449e796 Compare March 8, 2024 15:50

kokosing marked this pull request as ready for review March 8, 2024 15:51

dominikzalewski approved these changes Mar 8, 2024

View reviewed changes

kokosing force-pushed the origin/master/004 branch from 449e796 to f86153c Compare March 8, 2024 20:23

electrum reviewed Mar 8, 2024

View reviewed changes

kokosing force-pushed the origin/master/004 branch from f86153c to 752c139 Compare March 12, 2024 15:42

hashhar approved these changes Mar 15, 2024

View reviewed changes

kokosing added 3 commits March 15, 2024 14:15

Use ClosingBinder

0d4f855

There is no logic difference here. This commit replaces existing code with usage of closing binder without changing any logic.

Fix leaking resources from guice modules

c2b5224

kokosing force-pushed the origin/master/004 branch from 752c139 to c2b5224 Compare March 15, 2024 13:17

kokosing merged commit d67576e into trinodb:master Mar 18, 2024
95 checks passed

github-actions bot added this to the 443 milestone Mar 18, 2024

mosabua mentioned this pull request Mar 21, 2024

Add Trino 443 release notes #21178

Merged

kokosing deleted the origin/master/004 branch April 10, 2024 05:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ClosingBinder #20960

Introduce ClosingBinder #20960

kokosing commented Mar 6, 2024 •

edited

Loading

kokosing commented Mar 6, 2024

dominikzalewski left a comment

dominikzalewski Mar 6, 2024

wendigo Mar 6, 2024

dominikzalewski Mar 6, 2024

kokosing Mar 8, 2024

dominikzalewski Mar 6, 2024

kokosing Mar 8, 2024

dominikzalewski Mar 6, 2024

kokosing Mar 8, 2024

lukasz-stec Mar 6, 2024

kokosing Mar 8, 2024

kokosing Mar 8, 2024

kokosing Mar 8, 2024

kokosing commented Mar 8, 2024

dominikzalewski left a comment

dominikzalewski Mar 8, 2024

kokosing Mar 8, 2024

electrum Mar 8, 2024

kokosing Mar 12, 2024

electrum Mar 8, 2024

hashhar Mar 15, 2024 •

edited

Loading

electrum Mar 15, 2024 •

edited

Loading

kokosing commented Mar 12, 2024

kokosing commented Mar 15, 2024

hashhar left a comment

kokosing commented Mar 15, 2024

hashhar commented Mar 15, 2024

findepi commented Apr 9, 2024

Introduce ClosingBinder #20960

Introduce ClosingBinder #20960

Conversation

kokosing commented Mar 6, 2024 • edited Loading

kokosing commented Mar 6, 2024

dominikzalewski 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing commented Mar 8, 2024

dominikzalewski 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

hashhar Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

electrum Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

kokosing commented Mar 12, 2024

kokosing commented Mar 15, 2024

hashhar left a comment

Choose a reason for hiding this comment

kokosing commented Mar 15, 2024

hashhar commented Mar 15, 2024

findepi commented Apr 9, 2024

kokosing commented Mar 6, 2024 •

edited

Loading

hashhar Mar 15, 2024 •

edited

Loading

electrum Mar 15, 2024 •

edited

Loading