oscerd commented on code in PR #27329:
URL: https://github.com/apache/camel/pull/27329#discussion_r4177157345
##########
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:
Right — my insertion landed between that javadoc and the method it
documents, leaving two blocks stacked and the alert-notification one describing
`createOnAcceptDeliverSmExchange`. Moved back above
`createOnAcceptAlertNotificationExchange` in `0d7ee32`, kept as-is rather than
deleted since it is the one public method of the three.
_Claude Code on behalf of oscerd_
##########
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:
Not a negligible edge case — this one is a leak my own change introduced, so
thank you for catching it. Fixed in `0d7ee32`.
`createSmppMessage(CamelContext, DeliverSm)` declares `throws Exception` and
parses the delivery receipt, so a malformed receipt from the SMSC reaches it.
Your reading of the control flow is exactly right: `onAcceptDeliverSm` catches,
reports and returns, but its `exchange` variable is still unassigned at that
point, so the `finally` I added in the previous commit cannot release anything.
And your point about *why* it is new is the part worth underlining. Before
this PR the endpoint handed out a plain `DefaultExchange` and the GC dealt with
it. Taking it from the consumer's factory turns the same path into one lost
pooled exchange per failed decode — so the previous commit introduced a narrow
leak while its own description argued this change was not about leaks.
I applied the guard to all three creation methods rather than just
`deliverSm`, since `dataSm` and the alert-notification path can throw unchecked
from the binding too. I used `catch (Exception e)` rather than `catch
(Exception | Error e)`: it covers the realistic cases, and catching `Error` to
return a pooled object seemed the wrong trade when the JVM is already failing.
Covered by `deliverSmReleasesItsExchangeWhenTheMessageCannotBeDecoded` — a
binding mocked to throw `IOException`, asserting the release still happens and
the handler is still told. With the guard removed it fails on `Wanted but not
invoked` while the other three keep passing.
_Claude Code on behalf of oscerd_
--
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]
