oscerd commented on code in PR #27329:
URL: https://github.com/apache/camel/pull/27329#discussion_r4184002094
##########
components/camel-smpp/src/main/java/org/apache/camel/component/smpp/MessageReceiverListenerImpl.java:
##########
@@ -142,28 +150,79 @@ public DataSmResult onAcceptDataSm(DataSm dataSm, Session
session) throws Proces
LOG.debug("Received a dataSm {}", dataSm);
MessageId newMessageId = messageIDGenerator.newMessageId();
- Exchange exchange = endpoint.createOnAcceptDataSm(dataSm,
newMessageId.getValue());
+ Exchange exchange = createOnAcceptDataSmExchange(dataSm,
newMessageId.getValue());
try {
- processor.process(exchange);
- } catch (Exception e) {
- exchange.setException(e);
- }
+ try {
+ processor.process(exchange);
+ } catch (Exception e) {
+ exchange.setException(e);
+ }
- if (exchange.getException() != null) {
- ProcessRequestException pre =
exchange.getException(ProcessRequestException.class);
- if (pre == null) {
- pre = new
ProcessRequestException(exchange.getException().getMessage(), 255,
exchange.getException());
+ if (exchange.getException() != null) {
+ ProcessRequestException pre =
exchange.getException(ProcessRequestException.class);
+ if (pre == null) {
+ pre = new
ProcessRequestException(exchange.getException().getMessage(), 255,
exchange.getException());
+ }
+ throw pre;
}
- throw pre;
- }
- return new DataSmResult(newMessageId, dataSm.getOptionalParameters());
+ return new DataSmResult(newMessageId,
dataSm.getOptionalParameters());
+ } finally {
+ consumer.releaseExchange(exchange, false);
+ }
}
public void setMessageIDGenerator(MessageIDGenerator messageIDGenerator) {
this.messageIDGenerator = messageIDGenerator;
}
+ /**
+ * 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);
Review Comment:
Good catch — I had not considered the transceiver path at all, and you are
right about what it does.
The second constructor resolves `this.consumer = route.getConsumer()`, so in
TRX mode the consumer belongs to the receiver route and `getFromEndpoint()`
moves from the SMPP endpoint to e.g. `direct://messageReceiver`. As you say,
alert notifications already did this, so the change makes the three paths
consistent rather than introducing an oddity — but it is user visible, so it is
now in the 4.23 upgrade guide alongside the `fromRouteId`/pooling note and the
deprecation.
Test added as you asked:
`transceiverModeUsesTheReceiverRouteConsumerAndKeepsTheSmppPattern` drives the
listener through the `(endpoint, messageReceiverRouteId)` constructor against a
real started route — constructed before `context.start()` so the
`StartupListener` resolves the consumer — and asserts both `fromEndpoint` and
the pattern.
_Claude Code on behalf of oscerd_
##########
components/camel-smpp/src/main/java/org/apache/camel/component/smpp/MessageReceiverListenerImpl.java:
##########
@@ -142,28 +150,79 @@ public DataSmResult onAcceptDataSm(DataSm dataSm, Session
session) throws Proces
LOG.debug("Received a dataSm {}", dataSm);
MessageId newMessageId = messageIDGenerator.newMessageId();
- Exchange exchange = endpoint.createOnAcceptDataSm(dataSm,
newMessageId.getValue());
+ Exchange exchange = createOnAcceptDataSmExchange(dataSm,
newMessageId.getValue());
try {
- processor.process(exchange);
- } catch (Exception e) {
- exchange.setException(e);
- }
+ try {
+ processor.process(exchange);
+ } catch (Exception e) {
+ exchange.setException(e);
+ }
- if (exchange.getException() != null) {
- ProcessRequestException pre =
exchange.getException(ProcessRequestException.class);
- if (pre == null) {
- pre = new
ProcessRequestException(exchange.getException().getMessage(), 255,
exchange.getException());
+ if (exchange.getException() != null) {
+ ProcessRequestException pre =
exchange.getException(ProcessRequestException.class);
+ if (pre == null) {
+ pre = new
ProcessRequestException(exchange.getException().getMessage(), 255,
exchange.getException());
+ }
+ throw pre;
}
- throw pre;
- }
- return new DataSmResult(newMessageId, dataSm.getOptionalParameters());
+ return new DataSmResult(newMessageId,
dataSm.getOptionalParameters());
+ } finally {
+ consumer.releaseExchange(exchange, false);
+ }
}
public void setMessageIDGenerator(MessageIDGenerator messageIDGenerator) {
this.messageIDGenerator = messageIDGenerator;
}
+ /**
+ * 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);
+ try {
+ exchange.setPattern(endpoint.getExchangePattern());
Review Comment:
Added, and your framing made me realise the `setPattern` is load-bearing
rather than defensive — I had written it only to preserve the old behaviour
without thinking about which endpoint the factory would use.
The comment now reads:
> the pattern has to be set explicitly: in transceiver mode the consumer
belongs to the receiver route, not to this SMPP endpoint, so the factory would
otherwise stamp that endpoint's pattern on the exchange
and `dataSm` points back to it rather than repeating it.
It is pinned by the new TRX test, which sets the SMPP endpoint to `InOut`
against a `direct:` receiver route that defaults to `InOnly`. Removing the
`setPattern` fails it with `expected: <InOut> but was: <InOnly>`, so the two
endpoints' patterns genuinely differ in that setup rather than it being a
theoretical distinction.
_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]
