Copilot commented on code in PR #7940:
URL: https://github.com/apache/ignite-3/pull/7940#discussion_r3044703956
##########
modules/client/src/main/java/org/apache/ignite/internal/client/io/netty/NettyClientMessageHandler.java:
##########
@@ -37,13 +37,22 @@ public void channelRead(ChannelHandlerContext ctx, Object
msg) {
/** {@inheritDoc} */
@Override
public void channelInactive(ChannelHandlerContext ctx) {
- connection(ctx).onDisconnected(null);
+ NettyClientConnection conn = connection(ctx);
+
+ if (conn != null) {
+ conn.onDisconnected(null);
+ }
Review Comment:
After this change, `connection(ctx)` is treated as nullable in
`channelInactive`/`exceptionCaught`, but `channelRead` still dereferences it
unconditionally. If a read ever happens before `ATTR_CONN` is set (same
scenario that caused the NPE here), this would still throw; additionally, any
dropped inbound `ByteBuf` would need an explicit release to avoid leaks. Align
the null-handling strategy across all handler callbacks (either guarantee the
attribute is set before reads, or guard `channelRead` as well).
##########
modules/client/src/main/java/org/apache/ignite/internal/client/io/netty/NettyClientMessageHandler.java:
##########
@@ -37,13 +37,22 @@ public void channelRead(ChannelHandlerContext ctx, Object
msg) {
/** {@inheritDoc} */
@Override
public void channelInactive(ChannelHandlerContext ctx) {
- connection(ctx).onDisconnected(null);
+ NettyClientConnection conn = connection(ctx);
+
+ if (conn != null) {
+ conn.onDisconnected(null);
+ }
}
/** {@inheritDoc} */
@Override
public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
- connection(ctx).onDisconnected(cause);
+ NettyClientConnection conn = connection(ctx);
+
+ if (conn != null) {
+ conn.onDisconnected(cause);
Review Comment:
`exceptionCaught` now silently drops the original failure when `ATTR_CONN`
is not set (conn == null). In that scenario the SSL handshake exception (or
other root cause) never reaches `ClientConnectionStateHandler`, so higher
layers may only observe a generic "Channel is closed" later, losing the
actionable error details. Consider persisting the throwable on the channel (or
creating/attaching the connection attribute earlier) so that the real cause is
delivered to the state handler once/if the connection object is created, or at
minimum propagate/log the exception instead of swallowing it when conn is null.
```suggestion
conn.onDisconnected(cause);
} else {
ctx.fireExceptionCaught(cause);
```
--
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]
