[
https://issues.apache.org/jira/browse/CAMEL-25308?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18123096#comment-18123096
]
Andrea Cosentino commented on CAMEL-25308:
------------------------------------------
PR opened: https://github.com/apache/camel/pull/27329
Both the deliverSm and dataSm paths now build their exchange through
Consumer.createExchange(false), carrying over the endpoint pattern and binding,
and all three inbound paths release in a finally. Three tests assert the
contract directly.
Note on the revert-check: putting either path back on the endpoint factory
fails all three tests, but on an NPE rather than on the verify, because a
mocked endpoint hands back no exchange. They are a regression guard; the verify
calls are what pin the contract when the code is correct.
Module suite 140 tests green.
_Claude Code on behalf of oscerd_
> camel-smpp: the deliverSm and dataSm paths create exchanges on the endpoint
> instead of the consumer, so pooling never applies and fromRouteId is unset
> ------------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: CAMEL-25308
> URL: https://issues.apache.org/jira/browse/CAMEL-25308
> Project: Camel
> Issue Type: Bug
> Components: camel-smpp
> Reporter: Andrea Cosentino
> Assignee: Andrea Cosentino
> Priority: Minor
>
> {{Endpoint.createExchange}} documents the rule explicitly:
> {quote}*Important:* Consumers should use {{Consumer#createExchange(boolean)}}
> to create an exchange for which the consumer received a message.{quote}
> {{MessageReceiverListenerImpl}} follows that rule in one of its three inbound
> paths and not in the other two.
> h2. The three paths disagree
> {{onAcceptAlertNotification}} is correct - it builds the exchange through the
> consumer and releases it afterwards:
> {code:java}
> public Exchange createOnAcceptAlertNotificationExchange(AlertNotification
> alertNotification) {
> Exchange exchange = consumer.createExchange(false);
> ...
> }
> ...
> consumer.releaseExchange(exchange, false);
> {code}
> {{onAcceptDeliverSm}} and {{onAcceptDataSm}} instead go through the endpoint:
> {code:java}
> exchange = endpoint.createOnAcceptDeliverSmExchange(deliverSm);
> ...
> Exchange exchange = endpoint.createOnAcceptDataSm(dataSm,
> newMessageId.getValue());
> {code}
> and both of those end in {{DefaultEndpoint.createExchange(pattern)}}, which
> is {{DefaultExchange.newFromEndpoint(this, pattern)}} - a plain exchange that
> never goes near the configured {{ExchangeFactory}}. Neither path releases the
> exchange either.
> h2. What it costs
> This is *not* a pool leak: because nothing is taken from the pool, nothing is
> lost from it. What is lost is:
> * *Exchange pooling never applies to received messages.*
> {{camel.main.exchange-factory=pooled}} has no effect on {{deliverSm}}, which
> is the path every received SMS takes, nor on {{dataSm}} - only on the
> comparatively rare {{alertNotification}}.
> * *{{fromRouteId}} is unset on every received message.*
> {{DefaultConsumer.createExchange}} does
> {{answer.getExchangeExtension().setFromRouteId(routeId)}}; the endpoint path
> does not, so anything reading {{Exchange.getFromRouteId()}} - MDC logging,
> tracing, management - sees null for inbound SMPP traffic.
> h2. Suggested fix
> Build both exchanges in {{MessageReceiverListenerImpl}} through
> {{consumer.createExchange(false)}}, the way the alert-notification path
> already does, carrying over the endpoint's {{ExchangePattern}} and the
> binding, and release them in a {{finally}} so the {{ProcessRequestException}}
> path releases too.
> The endpoint's {{createOnAcceptDeliverSmExchange}} and
> {{createOnAcceptDataSm}} are public, so they are left in place rather than
> removed.
> Found while auditing camel-smpp; the component had no behavioural Jira since
> 2023.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
