[GitHub] [kafka] ijuma commented on a diff in pull request #12228: KAFKA-13950: Fix resource leaks

[GitBox <gi...@apache.org>]

ijuma commented on code in PR #12228:
URL: https://github.com/apache/kafka/pull/12228#discussion_r944876379


##########
clients/src/main/java/org/apache/kafka/common/network/PlaintextChannelBuilder.java:
##########
@@ -54,12 +54,18 @@ public void configure(Map<String, ?> configs) throws KafkaException {
     @Override
     public KafkaChannel buildChannel(String id, SelectionKey key, int maxReceiveSize,
                                      MemoryPool memoryPool, ChannelMetadataRegistry metadataRegistry) throws KafkaException {
+        PlaintextTransportLayer transportLayer = null;
         try {
-            PlaintextTransportLayer transportLayer = buildTransportLayer(key);
-            Supplier<Authenticator> authenticatorCreator = () -> new PlaintextAuthenticator(configs, transportLayer, listenerName);
+            transportLayer = buildTransportLayer(key);
+            final PlaintextTransportLayer finalTransportLayer = transportLayer;
+            Supplier<Authenticator> authenticatorCreator = () -> new PlaintextAuthenticator(configs, finalTransportLayer, listenerName);
             return buildChannel(id, transportLayer, authenticatorCreator, maxReceiveSize,
                     memoryPool != null ? memoryPool : MemoryPool.NONE, metadataRegistry);
         } catch (Exception e) {
+            // Ideally these resources are closed by the KafkaChannel but this builder should close the resources instead
+            // if an error occurs due to which KafkaChannel is not created.
+            Utils.closeQuietly(transportLayer, "transport layer for channel Id: " + id);
+            Utils.closeQuietly(metadataRegistry, "metadataRegistry");

Review Comment:
   I looked into this a bit more and it's a bit odd how we're doing clean-up. `buildAndAttach` currently closes the socket channel and cancels the key. It also creates the selector channel metadata registry. Shouldn't that take care of closing the metadata registry since it actually creates it?
   
   With regards to the transport layer, I wonder if we could move the cleanup logic to `KafkaChannel` instead. We could change `KafkaChannel` to take a `TransportLayer` supplier instead (or add a static factory method) and then have it handle the cleanup (instead of doing it in every implementation of channel builder.
   
   



-- 
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