Copilot commented on code in PR #7733:
URL: https://github.com/apache/ignite-3/pull/7733#discussion_r2905146293
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientInboundMessageHandler.java:
##########
@@ -421,28 +421,34 @@ public void channelRead(ChannelHandlerContext ctx, Object
msg) {
// Each inbound handler in a pipeline has to release the received
messages.
var unpacker = new ClientMessageUnpacker(byteBuf);
- metrics.bytesReceivedAdd(byteBuf.readableBytes() +
ClientMessageCommon.HEADER_SIZE);
- switch (state) {
- case STATE_BEFORE_HANDSHAKE:
- state = STATE_HANDSHAKE_REQUESTED;
-
metrics.bytesReceivedAdd(ClientMessageCommon.MAGIC_BYTES.length);
- handshake(ctx, unpacker);
+ try {
+ metrics.bytesReceivedAdd(byteBuf.readableBytes() +
ClientMessageCommon.HEADER_SIZE);
- break;
+ switch (state) {
+ case STATE_BEFORE_HANDSHAKE:
+ state = STATE_HANDSHAKE_REQUESTED;
+
metrics.bytesReceivedAdd(ClientMessageCommon.MAGIC_BYTES.length);
+ handshake(ctx, unpacker);
- case STATE_HANDSHAKE_REQUESTED:
- // Handshake is in progress, any messages are not allowed.
- throw new IgniteException(PROTOCOL_ERR, "Unexpected message
received before handshake completion");
+ break;
- case STATE_HANDSHAKE_RESPONSE_SENT:
- assert clientContext != null : "Client context != null";
- processOperation(ctx, unpacker);
+ case STATE_HANDSHAKE_REQUESTED:
+ // Handshake is in progress, any messages are not allowed.
+ throw new IgniteException(PROTOCOL_ERR, "Unexpected
message received before handshake completion");
- break;
+ case STATE_HANDSHAKE_RESPONSE_SENT:
+ assert clientContext != null : "Client context != null";
+ processOperation(ctx, unpacker);
- default:
- throw new IllegalStateException("Unexpected state: " + state);
+ break;
+
+ default:
+ throw new IllegalStateException("Unexpected state: " +
state);
+ }
+ } catch (Throwable t) {
+ unpacker.close();
Review Comment:
Catching `Throwable` is very broad and can interfere with JVM `Error`s
(e.g., `OutOfMemoryError`). If the intent is to ensure cleanup for regular
failures, prefer catching `Exception`/`RuntimeException` and using a `finally`
block for closing. If you intentionally need `Throwable` for Netty safety,
consider ensuring `unpacker.close()` cannot mask the original failure (e.g.,
guard close with its own try/catch and add suppressed).
```suggestion
try {
unpacker.close();
} catch (Throwable closeEx) {
t.addSuppressed(closeEx);
}
```
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientInboundMessageHandler.java:
##########
@@ -421,28 +421,34 @@ public void channelRead(ChannelHandlerContext ctx, Object
msg) {
// Each inbound handler in a pipeline has to release the received
messages.
var unpacker = new ClientMessageUnpacker(byteBuf);
- metrics.bytesReceivedAdd(byteBuf.readableBytes() +
ClientMessageCommon.HEADER_SIZE);
- switch (state) {
- case STATE_BEFORE_HANDSHAKE:
- state = STATE_HANDSHAKE_REQUESTED;
-
metrics.bytesReceivedAdd(ClientMessageCommon.MAGIC_BYTES.length);
- handshake(ctx, unpacker);
+ try {
+ metrics.bytesReceivedAdd(byteBuf.readableBytes() +
ClientMessageCommon.HEADER_SIZE);
- break;
+ switch (state) {
+ case STATE_BEFORE_HANDSHAKE:
+ state = STATE_HANDSHAKE_REQUESTED;
+
metrics.bytesReceivedAdd(ClientMessageCommon.MAGIC_BYTES.length);
+ handshake(ctx, unpacker);
- case STATE_HANDSHAKE_REQUESTED:
- // Handshake is in progress, any messages are not allowed.
- throw new IgniteException(PROTOCOL_ERR, "Unexpected message
received before handshake completion");
+ break;
- case STATE_HANDSHAKE_RESPONSE_SENT:
- assert clientContext != null : "Client context != null";
- processOperation(ctx, unpacker);
+ case STATE_HANDSHAKE_REQUESTED:
+ // Handshake is in progress, any messages are not allowed.
+ throw new IgniteException(PROTOCOL_ERR, "Unexpected
message received before handshake completion");
- break;
+ case STATE_HANDSHAKE_RESPONSE_SENT:
+ assert clientContext != null : "Client context != null";
+ processOperation(ctx, unpacker);
- default:
- throw new IllegalStateException("Unexpected state: " + state);
+ break;
+
+ default:
+ throw new IllegalStateException("Unexpected state: " +
state);
+ }
+ } catch (Throwable t) {
+ unpacker.close();
+ throw t;
Review Comment:
Resource cleanup is only performed on the exceptional path. If
`handshake(...)` / `processOperation(...)` do not close the unpacker
themselves, the success path may still leak buffers. Consider making ownership
explicit by using a `finally` (or try-with-resources if applicable) to close
`unpacker` deterministically, and adjust `handshake/processOperation` to not
close it (so there's a single clear owner).
```suggestion
throw t;
} finally {
unpacker.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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
