Skip to content

JmsTemplate ignoring TTL on message level #24144

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cynicLT opened this issue Dec 5, 2019 · 20 comments
Closed

JmsTemplate ignoring TTL on message level #24144

cynicLT opened this issue Dec 5, 2019 · 20 comments
Assignees
Labels
in: messaging Issues in messaging modules (jms, messaging) status: invalid An issue that we don't feel is valid

Comments

@cynicLT
Copy link

cynicLT commented Dec 5, 2019

Affects: 5.1.x


On sending message to JMS expiration header is ignored by JmsTemplate. TTL may be set per message - Message.setJMSExpiration(long expiration). For now JmsTesmpate support expiration of message per producer - any message level TTL is ignored

	 /**
	 * Actually send the given JMS message.
	 * @param producer the JMS MessageProducer to send with
	 * @param message the JMS Message to send
	 * @throws JMSException if thrown by JMS API methods
	 */
	protected void doSend(MessageProducer producer, Message message) throws JMSException {
		if (this.deliveryDelay >= 0) {
			producer.setDeliveryDelay(this.deliveryDelay);
		}
		if (isExplicitQosEnabled()) {
			producer.send(message, getDeliveryMode(), getPriority(), getTimeToLive());
		}
		else {
			producer.send(message);
		}
	}

Suggestion: resolve TTL of message in following way:

  1. TTL per message - if not set then
  2. TTL per producer

Current JmsTemplate implementation checks for 2 ignoring message level.

Example of fix:

	 /**
	 * Actually send the given JMS message.
	 * @param producer the JMS MessageProducer to send with
	 * @param message the JMS Message to send
	 * @throws JMSException if thrown by JMS API methods
	 */
	protected void doSend(MessageProducer producer, Message message) throws JMSException {
		if (this.deliveryDelay >= 0) {
			producer.setDeliveryDelay(this.deliveryDelay);
		}
               
                producer.setTimeToLive(resolveTimeToLive(message));

		if (isExplicitQosEnabled()) {
			producer.setDeliveryMode(getDeliveryMode());
			producer.setPriority(getPriority());
		}

		producer.send(message);
	}

	private long resolveTimeToLive(Message message){
       		long messageTimeToLive = message.getJMSExpiration();

		if (messageTimeToLive > 0{
                	return  messageTimeToLive ;
		} else {
                	return  getTimeToLive());
		}
	}

PR could be created and provided.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 5, 2019
@petronis
Copy link

petronis commented Dec 6, 2019

Had the same issue with it. This would help a lot really strange that you can't set TTL on message level, only on JMSTemplate level.

@kasuparu
Copy link

kasuparu commented Dec 6, 2019

Nicely spotted!

@jhoeller
Copy link
Contributor

jhoeller commented Dec 6, 2019

Message.setJMSExpiration isn't meant to be used by clients, it's meant to be called internally by the broker when sending a message. With which JMS provider does it work for client purposes?

Quoting its javadoc: "This method is for use by JMS providers only to set this field when a message is sent. This message cannot be used by clients to configure the expiration time of the message."

@jhoeller jhoeller self-assigned this Dec 6, 2019
@jhoeller jhoeller added in: messaging Issues in messaging modules (jms, messaging) type: enhancement A general enhancement labels Dec 6, 2019
@jhoeller
Copy link
Contributor

jhoeller commented Dec 6, 2019

I suppose we could try to detect pre-populated JMSExpiration and also JMSDeliveryMode and JMSPriority and preserve those values, at least in "explicitQosEnabled" mode. In the default mode, we rather won't pass in any explicit settings but leave it entirely up to the JMS provider through calling MessageProducer.send(Message), also not populating Message.setTimeToLive before since that may override the provider's default behavior otherwise. Note that the JMS provider may still internally check Message.getJMSTimeToLive before applying any default of its own, we just don't want to interfere with this.

@cynicLT
Copy link
Author

cynicLT commented Dec 6, 2019

Like this?

 /**
	 * Actually send the given JMS message.
	 * @param producer the JMS MessageProducer to send with
	 * @param message the JMS Message to send
	 * @throws JMSException if thrown by JMS API methods
	 */
	protected void doSend(MessageProducer producer, Message message) throws JMSException {
		if (this.deliveryDelay >= 0) {
			producer.setDeliveryDelay(this.deliveryDelay);
		}
               
		if (isExplicitQosEnabled()) {
			producer.setDeliveryMode(getDeliveryMode());
			producer.setPriority(getPriority());
			producer.setTimeToLive(resolveTimeToLive(message));
		}

		producer.send(message);
	}

	private long resolveTimeToLive(Message message){
       		long messageTimeToLive = message.getJMSExpiration();

		if (messageTimeToLive > 0{
                	return  messageTimeToLive ;
		} else {
                	return  getTimeToLive());
		}
	}

@jhoeller
Copy link
Contributor

jhoeller commented Dec 6, 2019

Indeed, if we do this it would be along those lines, also covering JMSDeliveryMode and JMSPriority the same way. However, it's still unclear to me whether this is even valid (see my comment above).

@jhoeller
Copy link
Contributor

jhoeller commented Dec 6, 2019

FWIW, we would preferably keep using the overloaded MessageProducer.send variant with the explicit arguments, making it easier for JMS connection pools to pool the provider (since they'll differentiate by provider settings while being able to reuse a provider instance when only runtime parameters are used).

@cynicLT
Copy link
Author

cynicLT commented Dec 6, 2019

Sure. MessageProducer has explicit API to be called with TTL

@jhoeller
Copy link
Contributor

jhoeller commented Dec 6, 2019

That API is what we're using in "explicitQosEnabled" already, mostly for the aforementioned provider pooling impact, so we'd simply like to preserve that part.

What I meant is that the only use case for this proposal is user code that pre-populates the message header through Message.setJMSExpiration - which is invalid according to the JMS javadoc. Why else would we have to check Message.getJMSExpiration manually? Same for setJMSDeliveryMode and setJMSPriority, all those methods are meant to be called by the JMS provider, not by user code.

Am I missing something? Is there a specific JMS provider that suggests such pre-populated expiration headers and documents it as officially supported?

@cynicLT
Copy link
Author

cynicLT commented Dec 6, 2019

As example of such work - ActiveMQ implementation of JMS API. It basicly ignores message TTL cause using defaults from MessageProducer API. And it allows to set some very specific headers like delivery mode, priority or TTL.

@jhoeller
Copy link
Contributor

jhoeller commented Dec 6, 2019

I'm curious about use cases here. So where specifically would you set a time-to-live value at the message level when using it with JmsTemplate? In a MessageCreator? In a MessagePostProcessor? Are you literally just calling Message.setJMSExpiration there before letting JmsTemplate take over? Do these values differentiate per message type or even per individual message?

@cynicLT
Copy link
Author

cynicLT commented Dec 6, 2019

We do it per message in MessageCreator based on message type by calling setJMSExpiration

@jhoeller
Copy link
Contributor

jhoeller commented Dec 6, 2019

After some further research, I still haven't found any indication that such a user-level Message.setJMSExpiration call is valid. Even in ActiveMQ forums, I saw the recommendation to always use producer-level settings since message-level settings will get overwritten by the broker.

ActiveMQ does not seem to respect a Message.setJMSExpiration setting when passing the resulting message to plain MessageProducer.send(Message), so this isn't something to adapt to in our "explicitQosEnabled" mode either. It looks like we'd have to manually handle those headers in both modes, as per the original code suggestion above, but that seems wrong in that it would bake a non-idiomatic pattern into our JmsTemplate... a pattern that would not work with plain JMS at all.

This issue title makes it sound like a JmsTemplate-specific deficiency while it actually is a JMS-wide convention that such message-level TTL headers are being ignored when set by the user. Are you asking us to circumvent that rule as a JmsTemplate-specific feature? Am I missing something?

@jhoeller jhoeller removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 6, 2019
@cynicLT
Copy link
Author

cynicLT commented Dec 7, 2019

Yes - as a JmsTemplate feature. It is widely used to communicate with JMS thus adopting such behavior would be friendly for framework users. It could be enabled per configuration. In later releases it could be removed as Brokers would introduce such behavior ( as it is in RabbitMQ)

@snicoll
Copy link
Member

snicoll commented Dec 7, 2019

While I agree that part of the spec/api is confusing and has tripped numerous users,
I don’t think we should do that.

The javadoc is very specific and I am not keen to ignore it. It would also be weird that JmsTemplate would do something that all other compliant JMS clients wouldn’t.

@cynicLT
Copy link
Author

cynicLT commented Dec 7, 2019

@snicoll does it mean that JmsTemplate should rely on send(Message) method and broker would handle priority TTL and other features based on message? Current implementation not following such approach.

@snicoll
Copy link
Member

snicoll commented Dec 7, 2019

The TL;DR is that Message.setJMSExpiration is not meant to be called by users, per spec and per javadoc so we shouldn't be doing that. If you need to set the expiration, you need to call the dedicated send method that takes a timeToLive. With JmsTemplate that happens with QosSettings: when that's set, JmsTemplate calls that specific send method which is what you should be doing.

@cynicLT
Copy link
Author

cynicLT commented Dec 7, 2019

So there is now way with Spring set TTL per message only per template? In other case should integrate with JMS broker bypassing Spring?

@snicoll
Copy link
Member

snicoll commented Dec 7, 2019

I think we're running a little bit in circles here. We've already answered this question in a number of different ways. I don't think bypassing Spring is going to give you much as you'd have to call the send specific method and the expiration time is not read from the Message there either.

I understand you'd like us to do something about it but unfortunately we can't as the spec is very clear on that front.

@snicoll snicoll closed this as completed Dec 7, 2019
@snicoll snicoll added status: invalid An issue that we don't feel is valid and removed type: enhancement A general enhancement labels Dec 7, 2019
@jhoeller
Copy link
Contributor

jhoeller commented Dec 7, 2019

From my perspective, I would only really revisit our arrangement in JmsTemplate if such message-level TTL settings work with regular JMS but not with JmsTemplate.

Since regular JMS MessageProducer.send(Message) seems to ignore any such user-set headers on the message as well, even on ActiveMQ, we seem to be in line with not only the JMS spec but also common runtime behavior in actual JMS brokers. In that sense, JmsTemplate ignoring TTL at the message level is by design, for alignment with regular JMS. Otherwise somebody could create a bug report and claim that we mis-respect JMS headers that need to be ignored as per the JMS spec...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: messaging Issues in messaging modules (jms, messaging) status: invalid An issue that we don't feel is valid
Projects
None yet
Development

No branches or pull requests

6 participants