[GitHub] [kafka] C0urante commented on a diff in pull request #13168: Kafka 14565: Interceptor Resource Leak

["C0urante (via GitHub)" <gi...@apache.org>]

C0urante commented on code in PR #13168:
URL: https://github.com/apache/kafka/pull/13168#discussion_r1092632585


##########
clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java:
##########
@@ -414,34 +414,51 @@ private <T> T getConfiguredInstance(Object klass, Class<T> t, Map<String, Object
      * Configurable configure it using the configuration.
      *
      * @param key The configuration key for the class
-     * @param t The interface the class should implement
+     * @param t   The interface the class should implement
      * @return A configured instance of the class
      */
     public <T> T getConfiguredInstance(String key, Class<T> t) {
-        return getConfiguredInstance(key, t, Collections.emptyMap());
+        T configuredInstance = null;
+
+        try {
+            configuredInstance = getConfiguredInstance(key, t, Collections.emptyMap());
+        } catch (Exception e) {
+            maybeClose(configuredInstance, "AutoCloseable object constructed and configured during failed call to getConfiguredInstance");
+            throw e;
+        }
+
+        return configuredInstance;
     }
 
     /**
      * Get a configured instance of the give class specified by the given configuration key. If the object implements
      * Configurable configure it using the configuration.
      *
-     * @param key The configuration key for the class
-     * @param t The interface the class should implement
+     * @param key             The configuration key for the class
+     * @param t               The interface the class should implement
      * @param configOverrides override origin configs
      * @return A configured instance of the class
      */
     public <T> T getConfiguredInstance(String key, Class<T> t, Map<String, Object> configOverrides) {
         Class<?> c = getClass(key);
+        T configuredInstance = null;
 
-        return getConfiguredInstance(c, t, originals(configOverrides));
+        try {
+            configuredInstance = getConfiguredInstance(c, t, originals(configOverrides));
+        } catch (Exception e) {

Review Comment:
   If we do the try/catch here, it doesn't work; `configuredInstance` is guaranteed to be null in the catch block.
   
   I was thinking of something like this:
   ```java
           if (o instanceof Configurable) {
               try {
                   ((Configurable) o).configure(configPairs);
               } catch (Exception e) {
                   maybeClose(o, "AutoCloseable object constructed and configured during failed call to getConfiguredInstance");
                   throw e;
               }
           }
   ```
   
   being added to [these lines](https://github.com/apache/kafka/blob/7d61d4505a16f09b85f5eb37adeff9c3534ec02c/clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java#L406-L407) in the other variant of `getConfiguredInstance`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org