gnodet-bot commented on code in PR #27329:
URL: https://github.com/apache/camel/pull/27329#discussion_r4177001657
##########
components/camel-smpp/src/main/java/org/apache/camel/component/smpp/MessageReceiverListenerImpl.java:
##########
@@ -171,6 +183,41 @@ public void setMessageIDGenerator(MessageIDGenerator
messageIDGenerator) {
* @param alertNotification the received message from the SMSC
* @return a new exchange
*/
+ /**
Review Comment:
⚠️ The Javadoc block immediately above (lines 183-185 in the context)
documents `createOnAcceptAlertNotificationExchange`, which now lives 35 lines
further down at line 221. The insertion of the two new private methods orphaned
it.
Either move the Javadoc down to sit directly above
`createOnAcceptAlertNotificationExchange`, or delete it if the method's
signature is self-documenting enough.
##########
components/camel-smpp/src/main/java/org/apache/camel/component/smpp/MessageReceiverListenerImpl.java:
##########
@@ -171,6 +183,41 @@ public void setMessageIDGenerator(MessageIDGenerator
messageIDGenerator) {
* @param alertNotification the received message from the SMSC
* @return a new exchange
*/
+ /**
+ * Creates the exchange for a received {@code deliver_sm}.
+ * <p/>
+ * Built through {@link Consumer#createExchange(boolean)} rather than on
the endpoint, as
+ * {@link org.apache.camel.Endpoint#createExchange()} instructs: an
exchange a consumer received a message for has
+ * to come from the configured {@code ExchangeFactory}, or pooling never
applies to it and its {@code fromRouteId}
+ * is left unset.
+ *
+ * @param deliverSm the received message from the SMSC
+ * @return a new exchange
+ */
+ private Exchange createOnAcceptDeliverSmExchange(DeliverSm deliverSm)
throws Exception {
+ Exchange exchange = consumer.createExchange(false);
+ exchange.setPattern(endpoint.getExchangePattern());
+ exchange.setProperty(Exchange.BINDING, endpoint.getBinding());
+
exchange.setIn(endpoint.getBinding().createSmppMessage(endpoint.getCamelContext(),
deliverSm));
+ return exchange;
Review Comment:
💡 Minor: if `createSmppMessage` (which declares `throws Exception`) fails
here, the exchange obtained from `consumer.createExchange(false)` on line 198
is never released — the exception propagates to the caller's `catch (Exception
e)` on line 118, but at that point the `exchange` variable in
`onAcceptDeliverSm` was never assigned, so the `finally` block is never reached
for it.
With non-pooled exchanges this was harmless (GC'd), but now that the
exchange comes from the consumer's `ExchangeFactory`, a decode failure would
leak one pooled exchange.
A guarding try/finally inside this method would close the gap:
```java
private Exchange createOnAcceptDeliverSmExchange(DeliverSm deliverSm) throws
Exception {
Exchange exchange = consumer.createExchange(false);
try {
exchange.setPattern(endpoint.getExchangePattern());
exchange.setProperty(Exchange.BINDING, endpoint.getBinding());
exchange.setIn(endpoint.getBinding().createSmppMessage(endpoint.getCamelContext(),
deliverSm));
return exchange;
} catch (Exception | Error e) {
consumer.releaseExchange(exchange, false);
throw e;
}
}
```
Narrow edge case — happy to defer if you consider the risk negligible.
--
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]
