tabish121 commented on code in PR #4530:
URL: https://github.com/apache/activemq-artemis/pull/4530#discussion_r1248216712
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ProtocolHandler.java:
##########
@@ -243,6 +243,18 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf
in, List<Object> out) t
ctx.flush();
}
+ @Override
+ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
+ // We definitely want to log the failure
+
ActiveMQServerLogger.LOGGER.failureDuringProtocolHandshake(ctx.channel().remoteAddress().toString(),
cause);
+
+ // However, what else should be done here? I've laid out a few
options...
+ ctx.fireChannelInactive();
Review Comment:
You shouldn't need to do this directly unless there's something in there you
really need to run right now.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ProtocolHandler.java:
##########
@@ -243,6 +243,18 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf
in, List<Object> out) t
ctx.flush();
}
+ @Override
+ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
+ // We definitely want to log the failure
+
ActiveMQServerLogger.LOGGER.failureDuringProtocolHandshake(ctx.channel().remoteAddress().toString(),
cause);
+
+ // However, what else should be done here? I've laid out a few
options...
+ ctx.fireChannelInactive();
+ ctx.close(); // this is done frequently in other implementations of
exceptionCaught()
+ ctx.fireExceptionCaught(cause);
+ ctx.pipeline().remove(this); // this is done at the end of decode();
should we do this as well?
Review Comment:
If the outcome is terminal then you shouldn't need to remove this, depends
on the handler but in general the pipeline is going to get torn down if you
close the context.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ProtocolHandler.java:
##########
@@ -243,6 +243,18 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf
in, List<Object> out) t
ctx.flush();
}
+ @Override
+ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
+ // We definitely want to log the failure
+
ActiveMQServerLogger.LOGGER.failureDuringProtocolHandshake(ctx.channel().remoteAddress().toString(),
cause);
+
+ // However, what else should be done here? I've laid out a few
options...
+ ctx.fireChannelInactive();
+ ctx.close(); // this is done frequently in other implementations of
exceptionCaught()
+ ctx.fireExceptionCaught(cause);
Review Comment:
If you are fully handling the exception caught case you don't need to
forward this, but if there's another handler that is in the chain that you want
to see the event then you could forward it.
--
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]
