Re: [PR] IGNITE-26184 Fix client connector magic bytes Netty buffer leak [ignite-3]

[via GitHub]

Copilot commented on code in PR #6453:
URL: https://github.com/apache/ignite-3/pull/6453#discussion_r2285934938


##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientInboundMessageHandler.java:
##########
@@ -604,26 +603,27 @@ private void sendHandshakeResponse(ChannelHandlerContext 
ctx, ClientMessagePacke
         throw new UnsupportedAuthenticationTypeException("Unsupported 
authentication type: " + authnType);
     }
 
-    private void writeMagic(ChannelHandlerContext ctx) {
-        ctx.write(Unpooled.wrappedBuffer(ClientMessageCommon.MAGIC_BYTES));
-        metrics.bytesSentAdd(ClientMessageCommon.MAGIC_BYTES.length);
-    }
-
-    private void write(ClientMessagePacker packer, ChannelHandlerContext ctx) {
+    private void writeAndFlush(ClientMessagePacker packer, 
ChannelHandlerContext ctx) {
         var buf = packer.getBuffer();
         int bytes = buf.readableBytes();
 
         try {
             // writeAndFlush releases pooled buffer.
             ctx.writeAndFlush(buf);
         } catch (Throwable t) {
-            buf.release();
+            packer.close();
             throw t;
         }
 
         metrics.bytesSentAdd(bytes);
     }
 
+    private void writeWithMagic(ClientMessagePacker packer, 
ChannelHandlerContext ctx) {
+        ctx.write(Unpooled.wrappedBuffer(ClientMessageCommon.MAGIC_BYTES));

Review Comment:
   The magic bytes buffer created by `Unpooled.wrappedBuffer()` is not being 
released, which could cause a memory leak. Consider using `ctx.writeAndFlush()` 
or ensure the buffer is properly released.
   ```suggestion
           
ctx.writeAndFlush(Unpooled.wrappedBuffer(ClientMessageCommon.MAGIC_BYTES));
   ```



##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientInboundMessageHandler.java:
##########
@@ -604,26 +603,27 @@ private void sendHandshakeResponse(ChannelHandlerContext 
ctx, ClientMessagePacke
         throw new UnsupportedAuthenticationTypeException("Unsupported 
authentication type: " + authnType);
     }
 
-    private void writeMagic(ChannelHandlerContext ctx) {
-        ctx.write(Unpooled.wrappedBuffer(ClientMessageCommon.MAGIC_BYTES));
-        metrics.bytesSentAdd(ClientMessageCommon.MAGIC_BYTES.length);
-    }
-
-    private void write(ClientMessagePacker packer, ChannelHandlerContext ctx) {
+    private void writeAndFlush(ClientMessagePacker packer, 
ChannelHandlerContext ctx) {
         var buf = packer.getBuffer();
         int bytes = buf.readableBytes();
 
         try {
             // writeAndFlush releases pooled buffer.
             ctx.writeAndFlush(buf);
         } catch (Throwable t) {
-            buf.release();
+            packer.close();

Review Comment:
   [nitpick] Calling `packer.close()` in the catch block may be redundant since 
the packer might already be released by the failed `writeAndFlush()` operation. 
Consider checking if the buffer is still available before closing.
   ```suggestion
               // Only close if buffer is still available.
               if (buf.refCnt() > 0) {
                   packer.close();
               }
   ```



-- 
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: notifications-unsubscr...@ignite.apache.org

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