You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "jbertram (via GitHub)" <gi...@apache.org> on 2023/06/30 18:36:40 UTC
[GitHub] [activemq-artemis] jbertram opened a new pull request, #4530: ARTEMIS-4338 + a couple of related fixes
jbertram opened a new pull request, #4530:
URL: https://github.com/apache/activemq-artemis/pull/4530
(no comment)
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] tabish121 commented on a diff in pull request #4530: ARTEMIS-4338 + a couple of related fixes
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4530: ARTEMIS-4338 + a couple of related fixes
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4530:
URL: https://github.com/apache/activemq-artemis/pull/4530#discussion_r1253194747
##########
tests/smoke-tests/pom.xml:
##########
@@ -188,6 +188,13 @@
<version>${project.version}</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.activemq.tests</groupId>
+ <artifactId>integration-tests</artifactId>
+ <version>${project.version}</version>
+ <type>test-jar</type>
+ <scope>test</scope>
+ </dependency>
Review Comment:
PR raised: #4536. Were that merged you should be able to just remove this dep, as the module already depends on artemis-test-support.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr merged pull request #4530: ARTEMIS-4338 + a couple of related fixes
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr merged PR #4530:
URL: https://github.com/apache/activemq-artemis/pull/4530
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4530: ARTEMIS-4338 + a couple of related fixes
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4530:
URL: https://github.com/apache/activemq-artemis/pull/4530#issuecomment-1615054668
I need some help with the implementation of `org.apache.activemq.artemis.core.protocol.ProtocolHandler.ProtocolDecoder#exceptionCaught` so anybody with Netty know-how please speak up!
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4530: ARTEMIS-4338 + a couple of related fixes
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4530:
URL: https://github.com/apache/activemq-artemis/pull/4530#discussion_r1253360980
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ProtocolHandler.java:
##########
@@ -243,6 +243,12 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
ctx.flush();
}
+ @Override
+ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
+ ActiveMQServerLogger.LOGGER.failureDuringProtocolHandshake(ctx.channel().remoteAddress().toString(), cause);
+ ctx.close();
Review Comment:
Yep
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4530: ARTEMIS-4338 + a couple of related fixes
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4530:
URL: https://github.com/apache/activemq-artemis/pull/4530#discussion_r1253370626
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/server/impl/RemotingServiceImpl.java:
##########
@@ -582,7 +582,7 @@ public void connectionCreated(final ActiveMQComponent component,
public void addConnectionEntry(Connection connection, ConnectionEntry entry) {
connections.put(connection.getID(), entry);
if (AuditLogger.isResourceLoggingEnabled()) {
- AuditLogger.createdConnection(connection.getProtocolConnection().getProtocolName(), connection.getID(), connection.getRemoteAddress());
+ AuditLogger.createdConnection(connection.getProtocolConnection() == null ? null : connection.getProtocolConnection().getProtocolName(), connection.getID(), connection.getRemoteAddress());
Review Comment:
Having the local-address in the actual audit log also seems like it would be [more] useful here, as in the handshake failure case. Probably a separate change though.
##########
tests/smoke-tests/src/main/resources/servers/audit-logging2/log4j2.properties:
##########
@@ -30,6 +30,9 @@ logger.artemis_journal.level=INFO
logger.artemis_utils.name=org.apache.activemq.artemis.utils
logger.artemis_utils.level=INFO
+logger.StompConnection.name=org.apache.activemq.artemis.core.protocol.stomp.StompConnection
+logger.StompConnection.level=TRACE
+
Review Comment:
Is this still needed if the new tests moved to AuditLoggerResourceTest? (looks to use different server config)
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4530: ARTEMIS-4338 + a couple of related fixes
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4530:
URL: https://github.com/apache/activemq-artemis/pull/4530#discussion_r1252202278
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ProtocolHandler.java:
##########
@@ -243,6 +243,12 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
ctx.flush();
}
+ @Override
+ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
+ ActiveMQServerLogger.LOGGER.failureDuringProtocolHandshake(ctx.channel().remoteAddress().toString(), cause);
Review Comment:
Including the local address may make sense...broker has multiple acceptors, which can also potentially be multi-homed on top of that.
Could potentially drop the toString and leave it to the logger.
##########
tests/smoke-tests/src/main/resources/servers/audit-logging2/log4j2.properties:
##########
@@ -40,7 +40,7 @@ logger.audit_base = INFO, audit_log_file
logger.audit_base.name = org.apache.activemq.audit.base
logger.audit_base.additivity = false
-logger.audit_resource = OFF, audit_log_file
+logger.audit_resource = INFO, audit_log_file
Review Comment:
Related to other comment, the resource logger is enabled in the AuditLoggerResourceTest test config already.
##########
tests/smoke-tests/pom.xml:
##########
@@ -188,6 +188,13 @@
<version>${project.version}</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.activemq.tests</groupId>
+ <artifactId>integration-tests</artifactId>
+ <version>${project.version}</version>
+ <type>test-jar</type>
+ <scope>test</scope>
+ </dependency>
Review Comment:
I've slowly been trying to break all of the ugly chained test-jar dependencies, and related classpath pollutions coming with them, so rather than adding/restoring this one...
It looks like you added this to get to a STOMP test-client? The AMQP test-client lives in the _artemis-test-support_ module for sharing supporting systest helper bits (specifically, tests/artemis-test-support/src/main/java/org/apache/activemq/transport/amqp/client/), as does the CFUtil the new tests also use. Seems like moving the STOMP test-client alongside them would be nicer than this, assuming its simple.
EDIT: looks like the stomp client innards already use the transport bits from artemis-test-support, making it seem like an even more appropriate move. I'll put together a PR after lunch.
##########
tests/smoke-tests/src/test/java/org/apache/activemq/artemis/tests/smoke/logging/AuditLoggerTest.java:
##########
@@ -117,6 +121,56 @@ public void testAuditLog() throws Exception {
checkAuditLogRecord(true, "is sending a message");
}
+ @Test
+ public void testCoreConnectionAuditLog() throws Exception {
+ testConnectionAuditLog("CORE");
+ }
+
+ @Test
+ public void testAMQPConnectionAuditLog() throws Exception {
+ testConnectionAuditLog("AMQP");
+ }
+
+ @Test
+ public void testOpenWireConnectionAuditLog() throws Exception {
+ testConnectionAuditLog("OPENWIRE");
+ }
Review Comment:
Since these are considered 'resource' logs, they might be better in AuditLoggerResourceTest
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ProtocolHandler.java:
##########
@@ -243,6 +243,12 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
ctx.flush();
}
+ @Override
+ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
+ ActiveMQServerLogger.LOGGER.failureDuringProtocolHandshake(ctx.channel().remoteAddress().toString(), cause);
+ ctx.close();
Review Comment:
Since this is the important bit perhaps whacking it in a try-finally would make sense.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4530: ARTEMIS-4338 + a couple of related fixes
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #4530:
URL: https://github.com/apache/activemq-artemis/pull/4530#discussion_r1253399434
##########
tests/smoke-tests/src/main/resources/servers/audit-logging2/log4j2.properties:
##########
@@ -30,6 +30,9 @@ logger.artemis_journal.level=INFO
logger.artemis_utils.name=org.apache.activemq.artemis.utils
logger.artemis_utils.level=INFO
+logger.StompConnection.name=org.apache.activemq.artemis.core.protocol.stomp.StompConnection
+logger.StompConnection.level=TRACE
+
Review Comment:
I left this in by mistake.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/server/impl/RemotingServiceImpl.java:
##########
@@ -582,7 +582,7 @@ public void connectionCreated(final ActiveMQComponent component,
public void addConnectionEntry(Connection connection, ConnectionEntry entry) {
connections.put(connection.getID(), entry);
if (AuditLogger.isResourceLoggingEnabled()) {
- AuditLogger.createdConnection(connection.getProtocolConnection().getProtocolName(), connection.getID(), connection.getRemoteAddress());
+ AuditLogger.createdConnection(connection.getProtocolConnection() == null ? null : connection.getProtocolConnection().getProtocolName(), connection.getID(), connection.getRemoteAddress());
Review Comment:
I agree on both counts.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4530: ARTEMIS-4338 + a couple of related fixes
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #4530:
URL: https://github.com/apache/activemq-artemis/pull/4530#discussion_r1253329624
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ProtocolHandler.java:
##########
@@ -243,6 +243,12 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
ctx.flush();
}
+ @Override
+ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
+ ActiveMQServerLogger.LOGGER.failureDuringProtocolHandshake(ctx.channel().remoteAddress().toString(), cause);
+ ctx.close();
Review Comment:
You mean something like this?
```java
@Override
public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
try {
ActiveMQServerLogger.LOGGER.failureDuringProtocolHandshake(ctx.channel().remoteAddress().toString(), cause);
} finally {
ctx.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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org