For connections created outside the global field, use try-with-resour…#1667
For connections created outside the global field, use try-with-resour…#1667jeanouii merged 4 commits intoapache:mainfrom
Conversation
…ces to avoid leaks
There was a problem hiding this comment.
Pull request overview
Updates AMQP transport test code to ensure JMS Connection instances are reliably closed (especially on exceptions) by using try-with-resources, reducing the chance of resource leaks during test runs.
Changes:
- Wrap JMS
Connectionusage in try-with-resources blocks across multiple AMQP-related test classes. - Remove manual
close()/finallypatterns made redundant by try-with-resources (including a double-close case). - Adjust test structure (indentation / block scoping) to keep assertions within the resource lifecycle.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 14 comments.
| File | Description |
|---|---|
activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/JMSLargeMessageSendRecvTest.java |
Wraps per-test connection creation in try-with-resources to ensure closure. |
activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/JMSInteroperabilityTest.java |
Wraps OpenWire/AMQP connections in try-with-resources across multiple interop tests. |
activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/AmqpTransformerTest.java |
Wraps AMQP and OpenWire JMS connections in try-with-resources; removes redundant close logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/JMSLargeMessageSendRecvTest.java
Outdated
Show resolved
Hide resolved
| Message message = c.receive(1000); | ||
|
|
||
| assertTrue(message instanceof BytesMessage); | ||
| Boolean nativeTransformationUsed = message.getBooleanProperty("JMS_AMQP_NATIVE"); | ||
| assertTrue("Didn't use the correct transformation, expected NATIVE", nativeTransformationUsed); | ||
| assertEquals(DeliveryMode.PERSISTENT, message.getJMSDeliveryMode()); | ||
| assertEquals(7, message.getJMSPriority()); | ||
| assertTrue(message instanceof BytesMessage); | ||
| Boolean nativeTransformationUsed = message.getBooleanProperty("JMS_AMQP_NATIVE"); | ||
| assertTrue("Didn't use the correct transformation, expected NATIVE", nativeTransformationUsed); | ||
| assertEquals(DeliveryMode.PERSISTENT, message.getJMSDeliveryMode()); | ||
| assertEquals(7, message.getJMSPriority()); |
There was a problem hiding this comment.
c.receive(1000) can return null; assertTrue(message instanceof BytesMessage) will then fail with a generic message. Add an assertNotNull (with a helpful message) before the type assertion to make failures easier to diagnose.
activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/AmqpTransformerTest.java
Outdated
Show resolved
Hide resolved
activemq-amqp/src/test/java/org/apache/activemq/transport/amqp/JMSInteroperabilityTest.java
Show resolved
Hide resolved
| LOG.debug("Appclication Property not set by client: {}", propertyName); | ||
| fail("Appclication Property not set by client: " + propertyName); |
There was a problem hiding this comment.
Typo in log / failure message: "Appclication" should be "Application" (both the LOG message and fail(...) message).
| LOG.debug("Appclication Property set by client is: {}", propertyName); | ||
| if (!inbound.propertyExists(propertyName)) { |
There was a problem hiding this comment.
Typo in log message: "Appclication" should be "Application".
| LOG.debug("Appclication Property not set by client: {}", propertyName); | ||
| fail("Appclication Property not set by client: " + propertyName); |
There was a problem hiding this comment.
Typo in log / failure message: "Appclication" should be "Application" (both the LOG message and fail(...) message).
| assertEquals(DeliveryMode.PERSISTENT, message.getJMSDeliveryMode()); | ||
| assertEquals(7, message.getJMSPriority()); | ||
| assertTrue(message instanceof BytesMessage); | ||
| Boolean nativeTransformationUsed = message.getBooleanProperty("JMS_AMQP_NATIVE"); |
There was a problem hiding this comment.
The variable 'nativeTransformationUsed' is only assigned values of primitive type and is never 'null', but it is declared with the boxed type 'Boolean'.
| Boolean nativeTransformationUsed = message.getBooleanProperty("JMS_AMQP_NATIVE"); | |
| boolean nativeTransformationUsed = message.getBooleanProperty("JMS_AMQP_NATIVE"); |
| assertNotNull("Should have received a message", message); | ||
| LOG.info("Recieved message: {}", message); | ||
| assertTrue(message instanceof BytesMessage); | ||
| Boolean nativeTransformationUsed = message.getBooleanProperty("JMS_AMQP_NATIVE"); |
There was a problem hiding this comment.
The variable 'nativeTransformationUsed' is only assigned values of primitive type and is never 'null', but it is declared with the boxed type 'Boolean'.
| Boolean nativeTransformationUsed = message.getBooleanProperty("JMS_AMQP_NATIVE"); | |
| boolean nativeTransformationUsed = message.getBooleanProperty("JMS_AMQP_NATIVE"); |
| assertFalse("Didn't use the correct transformation, expected NOT to be NATIVE", nativeTransformationUsed); | ||
| assertEquals(DeliveryMode.PERSISTENT, message.getJMSDeliveryMode()); | ||
| assertTrue(message instanceof TextMessage); | ||
| Boolean nativeTransformationUsed = message.getBooleanProperty("JMS_AMQP_NATIVE"); |
There was a problem hiding this comment.
The variable 'nativeTransformationUsed' is only assigned values of primitive type and is never 'null', but it is declared with the boxed type 'Boolean'.
| Boolean nativeTransformationUsed = message.getBooleanProperty("JMS_AMQP_NATIVE"); | |
| boolean nativeTransformationUsed = message.getBooleanProperty("JMS_AMQP_NATIVE"); |
…/JMSLargeMessageSendRecvTest.java Co-authored-by: Copilot <[email protected]>
…/AmqpTransformerTest.java Co-authored-by: Copilot <[email protected]>
…/JMSInteroperabilityTest.java Co-authored-by: Copilot <[email protected]>
…ces to avoid leaks